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

Autoloading Aliases #3940

Closed
wants to merge 2 commits into from
Closed

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Nov 28, 2020

Description
This PR adds a new Autoloader property to the config file: $aliases. Aliasing will allow the autoloader to intercept a class request and return an aliased version instead on-the-fly. This will make it easier to transition framework classes with user intervention as well as to replace core classes with user-defined versions.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner MGatner changed the title Add support for lazy class alias loading Autoloading Aliases Nov 28, 2020
system/Autoloader/Autoloader.php Outdated Show resolved Hide resolved
system/Autoloader/Autoloader.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Member Author

MGatner commented Nov 28, 2020

Okay got the branch issue cleared up, ready for re-review!

@MGatner MGatner mentioned this pull request Nov 28, 2020
5 tasks
@lonnieezell
Copy link
Member

I understand why you want to do this, especially with the PSR7 stuff coming up. I guess I'm a little concerned this muddies up when to use this vs using a service. This also wouldn't make services nearly as explicit since you may call one class in the service, but it gets implicitly aliased in a config file. To me this adds a bit of confusion to the framework.

I'm curious to hear other's thoughts.

@MGatner
Copy link
Member Author

MGatner commented Nov 29, 2020

I would like to hear others' thoughts as well. I wouldn't consider this ideal but it provides two things that we don't have currently:

  1. A way to transition classes that need breaking changes (frequently needed)
  2. A way to replace core classes (frequently requested)

Services can kind of solve the first problem but then you're stuck with the new version in App, which works if you've transitioned to a new namespace but not really if you've tried to do an in-place swap.

@MGatner
Copy link
Member Author

MGatner commented Nov 29, 2020

Two alternatives that I see:

  1. We control aliases internally and make them conditional on a Config setting. This way it isn't "any class can be any class" but there are a few preset replacements we know are a possibility
  2. We loosen our major version restrictions and start building 5. I know it sucks but the number of BC issues that come up every week could really start piling up to a new version quickly.

@paulbalandan
Copy link
Member

My thoughts on this:

  1. Mark this feature as @internal and to be used only for the transitory updating of classes in compliance with PSR7. After the transition is complete, remove this feature with no promise of BC for those who used it.

--or--

  1. Have a set of classes allowed to use this feature so that we control misuse and abuse. Yes, a Config setting would be nice but something that is unreachable from users' tampering.

@MGatner
Copy link
Member Author

MGatner commented Nov 29, 2020

One more thing to think about: currently there is no way to replace system-defined services. I assume this is a mistake/bug, but it adds another layer of difficulty solved by aliasing.

@tangix
Copy link
Contributor

tangix commented Dec 2, 2020

I second that aliasing would add a layer of confusion. Personally I use Services all over the place and love the simplicity.

Regarding overriding framework classes, in my 4.0.4 project I have "fixed" the Redis TTL bug by creating my own RedisHandler and adding my own class to $validHandlers in app/Config/Cache.php. This allowed me to fix the problem without messing in vendor directory. Happy camper :-)

@MGatner
Copy link
Member Author

MGatner commented Dec 3, 2020

Thanks all for the feedback. It sounds like we want to stick with our current tools, and avoid adding layers of complexity that something like this aliasing introduces. Closing.

@MGatner MGatner closed this Dec 3, 2020
@MGatner MGatner mentioned this pull request Jan 15, 2021
5 tasks
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.

4 participants