Allow int value as ID comparison for module loader

[bembelimen]

Summary of Changes

Let's assume, you have an AJAX request (com_ajax) for a module and you do something like:

$id = $input->getInt('id');

$module = ModuleHelper::getModuleById($id);

then the $module will be empty, because $modules[$i]->id is a string.

There are now two ways to solve this: change the === to == or this solution.

Testing Instructions

Look for the ID of an existing module.
Open the index.php of a template and put in the following code:

$module = JModuleHelper::getModuleById(123);

print_r($module);

(replace the "123" with the ID of the module).

Expected result

The module is loaded.

Actual result

An empty module is loaded.

[brianteeman]

This is a new feature and new features are not being accepted for j3

[SharkyKZ]

It's a bug fix. Integer columns can return either strings or integers based on database and its configuration.

E.g. before patch passing an integer works on PostgreSQL but works on MySQL. Passing a string works on MySQL but fails on PostgreSQL. With patch all combinations work.

[SharkyKZ]

joomla-cms/libraries/src/Helper/ModuleHelper.php

Line 653 in a1651fd

* @param string $id The id of the module

Update doc block? This should really be integer, IMO.

[HLeithner]

if we change the api we have to deprecate the type first and yes it should be integer.

[brianteeman]

Therefore it can not be changed in J3

[HLeithner]

This Pr can be merged (should because it's a bug and doesn't change behavior) but yes in 3.9 we can't deprecated.

[mbabker]

Changing the method to accept a string or integer changes the documented contract (i.e. the @param annotation of the doc block in this case), type widening is a new feature and in a strictly typed API would be a B/C break (the current effective signature is function &getModuleById(string $id) and a type widening change would make the effective signature function &getModuleById($id)).

Changing the method to only accept an integer in a B/C break in the documented contract.

The only reason you might be able to justify the "doesn't change behavior" part of your argument is that Joomla code has historically done absolutely zero type checking and basically leaves it to developers to figure out they're passing incorrect data into an API when an implementation detail changes that results in code breaking because developers were not honoring the documented contract (i.e. every change in the Registry API because people think they can throw whatever the hell they want into functions and things just work).

Or, just say "screw Semantic Versioning" and go back to ye olden days of merging whatever you want whenever you want.

[HLeithner]

Changing the method to accept a string or integer changes the documented contract (i.e. the @param annotation of the doc block in this case), type widening is a new feature and in a strictly typed API would be a B/C break (the current effective signature is function &getModuleById(string $id) and a type widening change would make the effective signature function &getModuleById($id)).

Changing the method to only accept an integer in a B/C break in the documented contract.

The only reason you might be able to justify the "doesn't change behavior" part of your argument is that Joomla code has historically done absolutely zero type checking and basically leaves it to developers to figure out they're passing incorrect data into an API when an implementation detail changes that results in code breaking because developers were not honoring the documented contract (i.e. every change in the Registry API because people think they can throw whatever the hell they want into functions and things just work).

Or, just say "screw Semantic Versioning" and go back to ye olden days of merging whatever you want whenever you want.

what are you talking about? no body want's to change the api, this pr changes only internal the way 2 $vars will be compared and for Joomla 5 (or 4 if we bring a 3.10 feature release) we change the parameter type to integer including deprecation warning.

[mbabker]

this pr changes only internal the way 2 $vars will be compared

As noted in the original issue body, the getModuleById() method is being called with an integer, which goes against the documented API contract. The developer is in the wrong for calling a method with an unsupported data type.

This pull request implicitly changes the documented API contract from only accepting a string to accepting either a string or an integer. Type widening of a method is not a bug fix, it is a new feature. If you want to accept this pull request (which there is not much of a reason not to), then per Semantic Versioning it is not acceptable for this change to land in a patch release. Because of the lack of typehints in the API, this change can safely land in a minor release, but this patch should be expanded to include inline type checks because for crying out loud the API needs to stop accepting anything thrown at it.

[mbabker]

Coming back with one unintended side effect introduced with this PR, with this patch as is one can legally do ModuleHelper::getModuleById(true); and the code will try to fetch the module with ID 1. Surely that's not intended. This is why type related behaviors need to be scrutinized carefully, especially when it deals with potential type widening changes or internal implementation details that change the type of an injected value.

[HLeithner]

@bembelimen michael is right, it doesn't make any sense to cast anything to int.

@mbabker would you say a check on is_numeric would work?

[mbabker]

That check is fair. It still supports the incorrect int type but also adds a level of validation against a bad call with a word based string.

I still think it would be OK to change the param type in 4.0 to string or int (with proper type checking and Exception if invalid) and a roadmap to deprecating string params (and of the type checking is added then the int casts this PR adds are acceptable). Clearly the passed param should be an int so moving the API that way is a good move.

[SharkyKZ]

This is wrong.

[mbabker]

How so?

[SharkyKZ]

Both values must be cast as the same type.

[bembelimen]

$modules[$i]->id is already a string (that's the reason of this PR)

[SharkyKZ]

No, it's returned from database. It can be either a string or an integer.

[bembelimen]

@mbabker thanks for your suggestions and improvements.
Didn't you post another improvement? I can't find the comment anymore.

[SharkyKZ]

@bembelimen Are you going to update this PR?

[SharkyKZ]

Regardless of where this is going, here's a PR for fixing PostgreSQL #28278.

[alikon]

as #28278 has been merged can we close this one @bembelimen ?