Skip to content

[4.0] Deleting duplicate module parameters#23553

Closed
Hackwar wants to merge 10 commits intojoomla:4.0-devfrom
Hackwar:j4modules
Closed

[4.0] Deleting duplicate module parameters#23553
Hackwar wants to merge 10 commits intojoomla:4.0-devfrom
Hackwar:j4modules

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented Jan 15, 2019

In Joomla 1.5, these parameters were in the default XML that com_modules loads. Then someone decided in 1.6 that it would be better to remove them from the default XML and instead copy all of them into each and every modules own XML file. Then in 3.0 we again added such parameters to the default XML. Sounds totally logical to me...

Anyway, this removes all that duplicate code and moves these duplicate parameters back into the advanced.xml. We should consider moving the content from advanced.xml into module.xml and moduleadmin.xml, where the caching would be dropped for the admin modules. (Caching is always disabled for logged in users anyway...) But that is something for another PR.

This has a downside, that some parameters might not be supported by the module and thus not work. Since the caching is done in the ModuleHelper class and the moduleclass suffix is now done in the chrome, that leaves the layout. I would say that that is acceptable and would push third party devs to better adapt the Joomla style of writing modules to support this parameter.

Going through all of this also showed that a bunch of modules had wrong paramters for caching. This should fix that one, too.

How to test?

Edit a module, see the parameters under advanced. Apply the patch, check the parameters again and see that they are all present and accounted for.

@Bakual
Copy link
Contributor

Bakual commented Jan 15, 2019

Personally, I wouldn't move the layout parameter as there may well be 3rd party modules that don't support layouts. And I don't like parameters that don't work.

The module class makes sense to me, since we move the module class out of the module to be only in the chrome. Which means it's supported as long as the template (and not the module) supports it.

The cache makes sense if it works even if the modules don't have any code for it. I honestly don't know if the modulehelper has some default behavior to use static caching if the module doesn't ask for something else.

@Hackwar
Copy link
Member Author

Hackwar commented Jan 15, 2019

So half the core modules have the parameter named 'cache', the other half has it named 'owncache'. I'm not messing around with that here, please someone decide which name to use.

@Hackwar
Copy link
Member Author

Hackwar commented Jan 23, 2019

I've rearranged the parameters a bit by having the identical parameters in a module.xml and the additional, frontend specific parameters in a site.xml, dropping the advanced.xml.

<field
name="language"
type="contentlanguage"
type="language"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??

Copy link
Member

@infograf768 infograf768 Jan 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong! It is OK for admin modules but NOT for site modules where the choice depends on existing Content Languages and not Installed languages.

Note: We also have to improve this for admin modules indeed as the language field is only displayed in function of the language filter enabled.
Admin modules filtering by language should be implemented independently of the Content Languages and depending on the availability of admin languages.

EDIT: I remarked you have set 'contentlanguage' as type for site.xml. But the issue is that it does not work here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the error is in

if ($clientId == 1)
{
$form = $this->loadForm('com_modules.module.admin', 'module', array('control' => 'jform', 'load_data' => $loadData), true);
// Display language field to filter admin custom menus per language
if (!ModuleHelper::isAdminMultilang())
{
$form->setFieldAttribute('language', 'type', 'hidden');
}
}
else
{
$form = $this->loadForm('com_modules.module', 'module', array('control' => 'jform', 'load_data' => $loadData), true);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what your issue is. It displays the language with the content languages for frontend modules when enabling multilang support and otherwise hides it.

@infograf768
Copy link
Member

infograf768 commented Feb 21, 2019

The language field for frontend modules depends on the enabling of the languagefilter plugin.
The type HAS to be contentlanguage and never language.
The language field display and use for admin modules depends on the parameter set in com_modules general Options.
It uses language as it is NOT related to Content Languages at all but only installed languages.

For example, on a multilingual site where 4 languages are installed, if one deletes (Trash and Empty Trash) one Content Language BUT does not uninstall the language, then that language will still display in the site module language field.
Here I deleted the Persian content Language and this is the language field of a site module which I get after your patch:
screen shot 2019-02-21 at 18 24 09

@infograf768
Copy link
Member

I have tested this item 🔴 unsuccessfully on 6e90789

For the reason stated above.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23553.

@Hackwar
Copy link
Member Author

Hackwar commented Feb 21, 2019

Fixed it

@Hackwar
Copy link
Member Author

Hackwar commented Feb 26, 2019

Ok, so since the parameters are already identical for backend and frontend, I've changed this around a bit to make this clearer. To repeat: Right now (without this PR) we already have the same parameters in the frontend and backend. This PR cleans up the code a bit and moves the parameters that are identical across all modules into the common parameters file for all modules.

@infograf768
Copy link
Member

Did not test all aspects, but my main remarks above are now solved. :)

@Hackwar
Copy link
Member Author

Hackwar commented Feb 26, 2019

Could you then change your test result?

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Mar 7, 2019

This doesn't go well with #23174, #23166, #23165 and #23152.

@ghost ghost added the J4 Issue label Apr 5, 2019
@ghost ghost removed the J4 Issue label Apr 13, 2019
@infograf768
Copy link
Member

Closing as it does not take into account modules not using cache.
Can be reopened if desired after correction.

@infograf768 infograf768 closed this Jul 8, 2019
@Hackwar Hackwar deleted the j4modules branch October 23, 2020 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants