? ? Pending

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
27 Feb 2023

The code to normalise the plugin name is intended to remove the plg_ from the start of the name
the same for components and modules

plg_system_example
==>
system_example

However str_replace removes all instances of plg_
plg_system_plg_example
==>
system_example

This PR changes the code to use str_starts_with which allows us to only remove the first instance. Note for phpn <8 it uses a polyfill

plg_system_plg_example
==>
system_plg_example

Pull Request for Issue #38254

Probably just testing by code review

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 27 Feb 2023
Category Libraries
avatar brianteeman brianteeman - open - 27 Feb 2023
avatar brianteeman brianteeman - change - 27 Feb 2023
Status New Pending
avatar Fedik
Fedik - comment - 27 Feb 2023

This should be complex fix in ExtensionManagerTrait class, for all: component, modules, plugin.
We should not merge only for plugin.

avatar brianteeman brianteeman - change - 27 Feb 2023
Labels Added: ?
avatar brianteeman
brianteeman - comment - 27 Feb 2023

This should be complex fix in ExtensionManagerTrait class, for all: component, modules, plugin. We should not merge only for plugin.

we only normalize plugins

avatar Fedik
Fedik - comment - 27 Feb 2023

No, we should fix all, check brianteeman#435
Because it affects names like xxxxcom_xxxx, or xxxxmod_xxx also.

For testing: apply patch and make sure all worked as before.

avatar brianteeman
brianteeman - comment - 27 Feb 2023

my bad. I didn't see the others

avatar brianteeman brianteeman - change - 27 Feb 2023
Title
[4.2] Normalise the plugin name
[4.2] Normalise extension names
avatar brianteeman brianteeman - edited - 27 Feb 2023
avatar brianteeman brianteeman - change - 27 Feb 2023
The description was changed
avatar brianteeman brianteeman - edited - 27 Feb 2023
avatar Fedik Fedik - test_item - 27 Feb 2023 - Tested successfully
avatar Fedik
Fedik - comment - 27 Feb 2023

I have tested this item successfully on b620c75

By review, looks good.


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

avatar HLeithner
HLeithner - comment - 27 Feb 2023

Thanks brian for this PR it's really appreciated. But @PhilETaylor informed the JSST that his PR is not compatible with php 7.2.5 which is true since str_starts_with is only available with PHP 8.

Could you please rebase it on Joomla 5.0 branch?

avatar richard67
richard67 - comment - 27 Feb 2023

Thanks brian for this PR it's really appreciated. But @PhilETaylor informed the JSST that his PR is not compatible with php 7.2.5 which is true since str_starts_with is only available with PHP 8.

Could you please rebase it on Joomla 5.0 branch?

@HLeithner As @Fedik pointed out in one of the resolved review comments above, we have a polyfill for it: /libraries/vendor/symfony/polyfill-php80

avatar HLeithner
HLeithner - comment - 27 Feb 2023

@HLeithner As @Fedik pointed out in one of the resolved review comments above, we have a polyfill for it: /libraries/vendor/symfony/polyfill-php80

hmm true, didn't saw it because the conversation was already "marked as solved". In this case I think we can keep it as it is. except afaik 4.2 only get bugfixes.

avatar richard67
richard67 - comment - 27 Feb 2023

hmm true, didn't saw it because the conversation was already "marked as solved". In this case I think we can keep it as it is. except afaik 4.2 only get bugfixes.

@HLeithner It fixes issue #38254 , which is labelled as a bug.

avatar HLeithner
HLeithner - comment - 27 Feb 2023

hmm true, didn't saw it because the conversation was already "marked as solved". In this case I think we can keep it as it is. except afaik 4.2 only get bugfixes.

@HLeithner It fixes issue #38254 , which is labelled as a bug.

In this case sorry for the noise...

avatar brianteeman
brianteeman - comment - 27 Feb 2023

In this case sorry for the noise...

No problem

avatar richard67
richard67 - comment - 27 Feb 2023

@brianteeman Now it only needs to adjust the description of the PR:

This PR changes the code to use preg_replace which allows us to only remove the first instance

avatar brianteeman brianteeman - change - 27 Feb 2023
The description was changed
avatar brianteeman brianteeman - edited - 27 Feb 2023
avatar brianteeman
brianteeman - comment - 27 Feb 2023

@brianteeman Now it only needs to adjust the description of the PR:

This PR changes the code to use preg_replace which allows us to only remove the first instance

done

avatar laoneo
laoneo - comment - 27 Feb 2023

But it should definitely be documented as the function supports now more patterns.

avatar richard67
richard67 - comment - 27 Feb 2023

We might also have to add the polyfill for 8.0 here: https://github.com/joomla/joomla-cms/blob/4.2-dev/composer.json#L79-L85

avatar brianteeman
brianteeman - comment - 27 Feb 2023

But it should definitely be documented as the function supports now more patterns.

No it just makes it work as intended

avatar Fedik
Fedik - comment - 28 Feb 2023

We might also have to add the polyfill for 8.0 here

I will do it in another PR, later, if will not forget ;)

avatar joomdonation joomdonation - test_item - 28 Feb 2023 - Tested successfully
avatar joomdonation
joomdonation - comment - 28 Feb 2023

I have tested this item successfully on b620c75

Code review + navigate to random pages in frontend and backend, nothing broken.


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

avatar joomdonation joomdonation - change - 28 Feb 2023
Status Pending Ready to Commit
avatar joomdonation
joomdonation - comment - 28 Feb 2023

RTC. Thanks !


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

avatar HLeithner
HLeithner - comment - 28 Feb 2023

@Fedik please make the composer pr and add the polyfill because now we use a function we (theoretically) may not have in the cms...

avatar Fedik
Fedik - comment - 28 Feb 2023

There a PR for composer #39968

avatar roland-d roland-d - change - 28 Feb 2023
Labels Added: ?
avatar fancyFranci fancyFranci - close - 28 Feb 2023
avatar fancyFranci fancyFranci - merge - 28 Feb 2023
avatar fancyFranci fancyFranci - change - 28 Feb 2023
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-02-28 21:41:17
Closed_By fancyFranci
avatar fancyFranci
fancyFranci - comment - 28 Feb 2023

Thanks a lot for your PR!

Add a Comment

Login with GitHub to post a comment