User tests: Successful: Unsuccessful:
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
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
Category | ⇒ | Libraries |
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
This should be complex fix in
ExtensionManagerTrait
class, for all: component, modules, plugin. We should not merge only for plugin.
we only normalize plugins
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.
my bad. I didn't see the others
Title |
|
I have tested this item
By review, looks good.
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?
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
@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.
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.
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...
In this case sorry for the noise...
No problem
@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
@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
But it should definitely be documented as the function supports now more patterns.
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
But it should definitely be documented as the function supports now more patterns.
No it just makes it work as intended
We might also have to add the polyfill for 8.0 here
I will do it in another PR, later, if will not forget ;)
I have tested this item
Code review + navigate to random pages in frontend and backend, nothing broken.
Status | Pending | ⇒ | Ready to Commit |
RTC. Thanks !
Labels |
Added:
?
|
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 |
Thanks a lot for your PR!
This should be complex fix in
ExtensionManagerTrait
class, for all: component, modules, plugin.We should not merge only for plugin.