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

fix: [Autoloader] Composer classmap usage #5850

Merged
merged 8 commits into from
Apr 3, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Apr 2, 2022

Description
The current implementation uses Composer classmap only when $modules->discoverInComposer is true.
discoverInComposer is the setting for Auto-Discovery within Composer Packages.
It has nothing to do with classmap usage.

This PR makes it use always if Composer is available.

Related: #5834

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Apr 2, 2022
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more along the lines of what I expected of the interaction between Autoloader, Composer, and Modules. I'm approving but let's be sure to hear @paulbalandan on this bc I am not entirely sure my understanding was correct. I also have never turned off Composer discovery so I'm not sure what people are expecting from that. Maybe @lonnieezell will have some insight from earlier development of these features?

@kenjis kenjis changed the title fix: [Autoloader] Composer classmap useage fix: [Autoloader] Composer classmap usage Apr 2, 2022
system/Autoloader/Autoloader.php Outdated Show resolved Hide resolved
system/Autoloader/Autoloader.php Outdated Show resolved Hide resolved
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the time to re-read the thread from my PR up to here and somehow it cleared the misconception I had from reading only the code. I think this approach, coupled with #5849, is better than mine. This PR now syncs the code with the documentation. 👍

Some technical notes:

  1. I would like the new methods to be internal only (i.e. private) to reduce future BC concerns. And I believe these methods would never be needed to be overridden by users.
  2. Annoying as it can be, let's just deprecate discoverComposerNamespaces() instead of removing. We can cleanup in v5.

@kenjis
Copy link
Member Author

kenjis commented Apr 2, 2022

@MGatner

I also have never turned off Composer discovery so I'm not sure what people are expecting from that.

If we have a lot of Composer packages, and not CodeIgniter packages, Composer discovery needs to look for files in these packages and it find nothing.

In that case, if we turn off Composer discovery and set the CodeIgniter Composer package namespaces in the Autoloader Config file, we can avoid the useless Composer discovery.

I thought that, but I have never tried yet.

@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Apr 2, 2022
@kenjis
Copy link
Member Author

kenjis commented Apr 2, 2022

@MGatner @mostafakhudair @paulbalandan
Thank you for the reviews.

I updated all the things, and added changelog.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all! Great collaboration.

@kenjis kenjis merged commit 3b8399b into codeigniter4:develop Apr 3, 2022
@kenjis kenjis deleted the refactor-autoloader branch April 3, 2022 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants