Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.2] Load the namespace from the cached manifest #44737

Merged
merged 5 commits into from
Jan 18, 2025

Conversation

HLeithner
Copy link
Member

Summary of Changes

Set the namespace in componentHelper::getComponent() result.

Testing Instructions

Use an namespace extension, execute var_dump(componentHelper::getComponent('com_xxx');

Actual result BEFORE applying this Pull Request

$namespace is always null

Expected result AFTER applying this Pull Request

$namespace is filled if extension has an namespace

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

@Hackwar Hackwar added the bug label Jan 17, 2025
@joomdonation
Copy link
Contributor

I have tested this item ✅ successfully on 98907ee


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

1 similar comment
@rdeutz
Copy link
Contributor

rdeutz commented Jan 18, 2025

I have tested this item ✅ successfully on 98907ee


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

@rdeutz rdeutz removed the bug label Jan 18, 2025
@rdeutz
Copy link
Contributor

rdeutz commented Jan 18, 2025

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 18, 2025
@richard67 richard67 enabled auto-merge (squash) January 18, 2025 11:10
@richard67 richard67 added this to the Joomla! 5.2.4 milestone Jan 18, 2025
@Fedik
Copy link
Member

Fedik commented Jan 18, 2025

I am sorry to say, but this should not be merged.
With this we pulling few extra bites of manifest_cache for all components on each request for nothing.

$namespace is always null

It is design flaw. Can just remove that as useles property :)

If we need this feature it should be proper API that will cover all extension.
IDK, namespace column in #__extensions or something to read it from extension service provider.

@rdeutz
Copy link
Contributor

rdeutz commented Jan 18, 2025

I am not sure why it is needed but it is one field more in the queue that is already running and it sets a propery that is also there. So if this is not needed then we should remove the property and not let it NULL.

@richard67 richard67 disabled auto-merge January 18, 2025 11:40
@HLeithner
Copy link
Member Author

that's the point an empty variable doesn't make sense and in this I need it so I thought it was simply for forgotten.

@joomdonation
Copy link
Contributor

Sorry, I was aware that we do not use it in core at the moment, but didn't think carefully enough about few extra bytes on each request for nothing which @Fedik point out. For this, I agree with @Fedik


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

@HLeithner
Copy link
Member Author

Ok, then please tell me a better solution to get a namespace of any extension.

@Fedik
Copy link
Member

Fedik commented Jan 18, 2025

please tell me a better solution to get a namespace of any extension

In theory we should not need to know the namespace, however I agree in some cases it is usefull.
As for now I have no idea.
Maybe kind of $app->bootExtension('potato')->getNamespace() which is aslo bad because that is doing whole extension initialisation.

If I would need it I would just do some little helper for myself that read it directly from XML, as it done in https://github.com/joomla/joomla-cms/blob/5.2-dev/libraries/namespacemap.php

Or maybe as part of JNamespacePsr4Map kind of (new JNamespacePsr4Map)->getNamespace('component', 'com_potato')

@HLeithner
Copy link
Member Author

ok then I see we have a function what exactly does your suggestion but you don't want it because creating a completely new (everyone at it's own) is better?

Sorry but that doesn't make sense for me. Even if you are right that the namespace in theory is not needed, but only if you are in the joomla context, is a bit different if you do strange things like prefixing 3rd party libraries.

To solve my issue I think I would only need a helperFactory in the component but to be honest that's something which is even worse ;-)

@Fedik
Copy link
Member

Fedik commented Jan 18, 2025

Well, that is not me who designed it ;)
We cannot even get extension service container and that even more confusing than unable to access to its namespace.

I think someone just forgot to remove the $namspace property of ComponentRecored when JNamespacePsr4Map was writen.

@HLeithner
Copy link
Member Author

I'm ok with deprecating this variable for the future if we have a better way but at the moment I see no other way coming to town right?

@Hackwar Hackwar merged commit ca1a8e6 into joomla:5.2-dev Jan 18, 2025
0 of 2 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 18, 2025
@Hackwar
Copy link
Member

Hackwar commented Jan 18, 2025

I'm siding with Harald on this one and thus merged it. Thank you! If you think this is wrong, lets please open a PR to deprecate this attribute.

@Fedik
Copy link
Member

Fedik commented Jan 19, 2025

What a horrible mistake you guys just did. In patch release.

@laoneo
Copy link
Member

laoneo commented Jan 20, 2025

Please guys, revert this pr. It brings us back to the ages where single classes will be instantiated. @Fedik is completely right, this needs to be removed and everything should go through the service provider.

HLeithner pushed a commit that referenced this pull request Jan 20, 2025
@QuyTon QuyTon removed this from the Joomla! 5.2.4 milestone Jan 20, 2025
@pe7er pe7er added this to the Joomla! 5.2.4 milestone Jan 21, 2025
@pe7er
Copy link
Contributor

pe7er commented Jan 21, 2025

I've re-added the 5.2.4 milestone. We've added this to the 5.2-dev branch for 5.2.4, and fixed it with #44755

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants