Joomla! Issue Tracker - CMS

Download
Launch
Feeling Lucky

[#12478] - Mark _autoload function as public and not private.

? Success

User tests: Successful: Unsuccessful:

avatar photodude
photodude
19 Oct 2016

Pull Request for Issue _autoload was marked private.

Why _autoload was ever marked private is highly questionable. Ever since the autoloader has been there, people have been filing issues because it is incompatible with third-party code that uses public functions.

My only assumption is _autoload got marked private by habit since in PHP 4 there was a preference to indicate methods private or protected with leading underscores and that resulted in any PHP 5 methods prefixed with underscores being explicitly marked private or protected.

In this instance, it seems like marking _autoload as private is the completely wrong thing to do. (it's also questionable why it even works in PHP facebook/hhvm#959 (comment) )

Summary of Changes

Explicitly mark _autoload and _load as public.

Testing Instructions

  • Everything should still work in Joomla

  • Review Travis CI HHVM tests, the JLoaderTest::testSetupWithoutPrefixes failure is no longer listed.

  • Optional extended testing

    • Previously incompatible third-party code that uses public functions should now autoload without issue.

Documentation Changes Required

Unsure about documentation changes
(possibly should add a note in the docblocks to explicitly state these methods need to be public.)

avatar photodude photodude - open - 19 Oct 2016
avatar photodude photodude - change - 19 Oct 2016
Status New ⇒ Pending
avatar joomla-cms-bot joomla-cms-bot - change - 19 Oct 2016
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 19 Oct 2016
Category ⇒ Libraries
avatar photodude photodude - change - 19 Oct 2016
The description was changed
avatar photodude photodude - edited - 19 Oct 2016
avatar csthomas
csthomas - comment - 19 Oct 2016

What is the reason for marking _load function as a public?

avatar photodude
photodude - comment - 19 Oct 2016

@csthomas I couldn't come up with a justification to leave it private. Additionally, since load is public and _autoload should have been public, it seems that _load also needed to be public.

Let me know if there is a reason to leave _load as private.

avatar mbabker
mbabker - comment - 19 Oct 2016

_load is only used internally by _autoload. For the compatibility issues the _autoload method needs to be exposed but I can't come up with a valid reason _load needs to be too.

avatar csthomas
csthomas - comment - 19 Oct 2016

IMHO If it stays private then in the future we can easily rename or remove it without B/C issue.

avatar photodude
photodude - comment - 19 Oct 2016

I can change it if that's desired.
I was just looking over the PSR-4 example protected seems more correct. The only methods in PSR-4 autoloader class example that were marked protected were ones that returned file paths or implemented a required file with require $file which would be like our include $path.

So should it left as private or changed to protected?

Although this reasoning probably doesn't apply to import(), load(), or loadByPsr0() (although I could be wrong)

avatar photodude
photodude - comment - 19 Oct 2016

Maybe it's out of scope for this, but any thoughts on include vs include_once in the context of the _load() method here? I'm leaning towards it being more correct to be include_once

avatar csthomas
csthomas - comment - 20 Oct 2016

When _autoload() is private then user can not run it manually and include is enough.
Now it will be public and user can run it manually twice which generates error but IMHO the error should be visible and we should stay with include only.

For me, you can move the method _load to inside _autoload and then you won't have any private method.

avatar photodude
photodude - comment - 20 Oct 2016

@csthomas it's been reverted back to private. Let's get this tested and merged.

avatar csthomas csthomas - test_item - 20 Oct 2016 - Tested successfully
avatar csthomas
csthomas - comment - 20 Oct 2016

I have tested this item ✅ successfully on 38e51c0

Joomla works normal.

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

avatar csthomas
csthomas - comment - 20 Oct 2016

I added success test. Should I add approved review too? Which is corrected?

avatar zero-24
zero-24 - comment - 20 Oct 2016

If you test thank just test via the tracker. If you review the code than review iirc :)

avatar photodude
photodude - comment - 20 Oct 2016

@csthomas as per @mbabker 's suggestion if you test you should not mark as approved review and vice versa. That way we get the full 4 reviews that are suggested, 2 testers and 2 reviewers who are a total of 4 different people.

avatar dgt41 dgt41 - test_item - 20 Oct 2016 - Tested successfully
avatar dgt41
dgt41 - comment - 20 Oct 2016

I have tested this item ✅ successfully on 38e51c0

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

avatar photodude photodude - edited - 20 Oct 2016
avatar zero-24 zero-24 - change - 20 Oct 2016
Milestone Added:
Status Pending ⇒ Ready to Commit
avatar zero-24
zero-24 - comment - 20 Oct 2016

RTC

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

avatar joomla-cms-bot joomla-cms-bot - change - 20 Oct 2016
Labels Added: ?
avatar brianteeman brianteeman - close - 25 Oct 2016
avatar rdeutz rdeutz - change - 25 Oct 2016
Status Ready to Commit ⇒ Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 ⇒ 2016-10-25 17:35:06
Closed_By ⇒ rdeutz
avatar rdeutz rdeutz - close - 25 Oct 2016
avatar rdeutz rdeutz - merge - 25 Oct 2016
avatar brianteeman brianteeman - change - 28 Oct 2016
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment