Skip to content

fix: [Autoloader] Composer classmap usage#5850

Merged
kenjis merged 8 commits into
codeigniter4:developfrom
kenjis:refactor-autoloader
Apr 3, 2022
Merged

fix: [Autoloader] Composer classmap usage#5850
kenjis merged 8 commits into
codeigniter4:developfrom
kenjis:refactor-autoloader

Conversation

@kenjis

@kenjis kenjis commented Apr 2, 2022

Copy link
Copy Markdown
Member

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
@samsonasik samsonasik requested a review from paulbalandan April 2, 2022 05:45

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

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
Comment thread system/Autoloader/Autoloader.php Outdated
Comment thread system/Autoloader/Autoloader.php Outdated
Comment thread system/Autoloader/Autoloader.php

@paulbalandan paulbalandan left a comment

Copy link
Copy Markdown
Member

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 kenjis force-pushed the refactor-autoloader branch from 7b03571 to c7e659f Compare April 2, 2022 23:14
@kenjis

kenjis commented Apr 2, 2022

Copy link
Copy Markdown
Member Author

@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

kenjis commented Apr 2, 2022

Copy link
Copy Markdown
Member Author

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

I updated all the things, and added changelog.

@kenjis kenjis requested review from MGatner and paulbalandan April 2, 2022 23:25

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

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