[4.0] Fix Modules Filter

[joomdonation]

Pull Request for Issue #33692.

Summary of Changes

Currently, we use same session key for Admin and Site modules filter options value, thus causes unexpected behavior. One of the issue is #33692

There are other issues, too. For example, if you are on Site Modules and select a module position filter, then switch to Administrator, you won't see any modules... (because the system remember that site position, but of course, backend modules have different positions list. Same for module filter and menu item filter.

Testing Instructions

1. Look at the issue [4.0] Backend: Admin modules use language filter of site modules #33692 , confirm that the issue happens (you would need to have a multilingual website to confirm the issue)
2. Apply patch, confirm that the issue sorted.
3. Try to filter modules by position, by module types and make sure it is still working as expected, nothing should be broken
4. From Modules Management screen, choose a module position. Then press New button in toolbar, confirm that that module position is pre-selected on New module screen.

[ghost]

I have tested this item 🔴 unsuccessfully on 16f40f9

Test using patchtester got same behaviour with and without pr. Test filter on language and position.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33763.}

[joomdonation]

Ah, no. The fix is not this simple. I will see if I can find a solution

[joomdonation]

Close this for now. When I come up with a solution, I will re-open.

[joomdonation]

@sandramay0905 Could you please give it another try?

[ghost]

I have tested this item ✅ successfully on f449246

Test Filters Site: Language, Position and combination > Admin display of modules is not effected.

Enabled on Admin language filtering: Filter on Persian and in site on english > works.

Thanks @joomdonation

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33763.}

[astridx]

Unfortunately, the context is hard coded in some places. These would have to be adapted with this solution.

One is here, for example:

joomla-cms/administrator/components/com_modules/src/Service/HTML/Modules.php

Line 190 in 92aa075

$position = $app->getUserState('com_modules.modules.filter.position');

If this is not changed, the following can happen:

1. Make a new installaiton and filter modules

2. Move a module that is the only one on its position to another position.
   

3. After moving you see no module in the list.
   

[joomdonation]

@astridx Do you know if there are other places affected by this change? I ask because you tried to fix this issue before, so you have more experience and could help.

[joomdonation]

@astridx Unless I'm mistaken, I think the issue you are talking about happens before this PR:

• The modules list empty because there are no modules left for that position (after the last module from that position moved to new position)
• What's wrong here is the system still remember the original position (because it is stored in the session), but the positions filter does not contain that position anymore (the positions filter from dropdown only contains positions which has at least one module assigned to it). Maybe we can modify code of this method https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_modules/src/Field/ModulesPositionField.php#L120 , add the missing position (which is stored in the session to the array, same with the code at https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_modules/src/Service/HTML/Modules.php#L188-L197

Something like (quick draft code, clean up needed)

$app = Factory::getApplication();

$position = $app->getUserState('com_modules.modules.' . $clientId . '.filter.position');

if ($position)
{
	$found = false;

	// Add active position here
	foreach ($options as $option)
	{
		if ($option->value ===$position)
		{
			$found = true;
			break;
		}
	}

	if (!$found)
	{
		$options[] = \JHtml::_('select.option', $position, $position);
	}
}

How do you think about it?

[ceford]

I have tested this item ✅ successfully on ca23363

I confirm that switching back and forth between Site and Administrator gives independent module lists.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33763.}

[astridx]

@astridx Unless I'm mistaken, I think the issue you are talking about happens before this PR:

Now I'm not sure I understand you correctly. Without this PR, the error described in #33763 (comment) does not occur, right?

@astridx Do you know if there are other places affected by this change?

I don't know Joomla well. I searched for "com_modules.modules" and found this place:

joomla-cms/administrator/components/com_modules/src/Model/ModuleModel.php

Line 642 in 9653158

$filters = (array) $app->getUserState('com_modules.modules.filter');

But I don't know how important this is and I don't know if there are other places where the context might be put together with variables?

[joomdonation]

Now I'm not sure I understand you correctly. Without this PR, the error described in #33763 (comment) does not occur, right?

Actually, that error happens because this PR. I explained the issue and proposed the solution in my earlier comment #33763 (comment). But I don't want to include that in this PR. I can make a new PR for that if needed.

I don't know Joomla well. I searched for "com_modules.modules" and found this place:

=> That's not affected by the PR. It uses to auto-populate data when you create new module base on data from filter. For example, if you choose a Position in Modules Management page, that will be default position when you press New button to create a new module. That still works after this PR.

[joomdonation]

@astridx Thinking more about the issue you described, I think after moving the modules to new position, we should set the position filter to that new position. That would be best solution, I think

[joomdonation]

But I don't know how important this is and I don't know if there are other places that might be put together with variables?

Actually, that is affected by this PR and fixed by this comment d47154e

[joomdonation]

@sandramay0905 @ceford Could you please help testing it again? There is a new commit which auto-populate data for New Module base on data from filter (step #4 in testing instructions). Compare to your previous test, that new step is needed.

The issue which @astridx mentioned is independent with this PR and I will make a new PR to fix it after this one merged (because the new code will depend on this PR)

Thanks.

[ceford]

I have tested this item ✅ successfully on d47154e

Confirmed that it works and step 4 works.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33763.}

[ghost]

I have tested this item ✅ successfully on d47154e

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33763.}

[joomdonation]

Thanks @ceford and @sandramay0905 for testing.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33763.}

[sonnemondundsterne]

Thanks astrid for finding possible issues.

[Stuartemk]

It is assumed that (array) should no longer be used, instead it should be [], there are many (array) in the whole Joomla 4 cms and they are still adding more, perhaps there is something that I do not know and justify the use of (array). Please tell me if I am in error and if not, change all the (array) to [] or at least not continue with that obsolete practice.
They are doing a great job !

[joomdonation]

@Stuartemk I don't understand your comment. Is this something related to this PR ?

[richard67]

@Stuartemk Maybe you mix up type cast (array) $somevariable and array creation array(1,2,3)? Only the latter is replaced by short array syntax [1,2,3] in J4, but here in the diff of this PR I only see a type cast to array, and that's something completely different.

[Stuartemk]

Maybe I'm confused, but I think this should be changed to be 100% compatible with php8 I think so, but I can be wrong

$filters = (array) $app->getUserState('com_modules.modules.' . $clientId . '.filter');

for this

$filters = [ ] $app->getUserState('com_modules.modules.' . $clientId . '.filter');

Regards

[joomdonation]

@Stuartemk No, it's not valid syntax. For type casting, we will still have to use the original code:

$filters = (array) $app->getUserState('com_modules.modules.' . $clientId . '.filter');

[richard67]

Nothing else than what I have said.

[Quy]

Thank you!