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

chore(autoloading): Only use authoritative classmaps for production #8020

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

ChristophWurst
Copy link
Member

How to test

  1. Run composer i
  2. Check vendor/composer/autoload_real.php

Main: you find a $loader->setClassMapAuthoritative(true); and switching between branches could potentially trigger missing classes if you don't run composer i or composer dump-autoload to update the class map
Here: class map is still optimized on dev but tries to look up missing classes. On production we use the authoritative classmaps nevertheless.

Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

I tried testing this with the following steps:

  1. Check out main
  2. Run composer i
  3. check out this branch
  4. create new empty migration via occ and bump info.xml
  5. run the upgrade from the web ui

Unfortunately the lookup didn't work with the typical error that happens when the composer autoloader wasn't dumped:

image

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works.


@miaulalala The existing autoloader has the authoritative state ingrained and thus it has to be generated once more after this PR. The setting from composer.json is only read on dumping the autoloader (or installing dependencies).

vendor/composer/autoload_real.php

  41   │         }
  42   │
  43   │         $loader->setClassMapAuthoritative(true);
  44   │         $loader->register(true);
  45   │
  46   │         if ($useStaticLoader) {

@ChristophWurst
Copy link
Member Author

Excellent catch. That explains the missing class despite my changes.

@ChristophWurst ChristophWurst marked this pull request as ready for review February 16, 2023 09:43
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

Tested again:

  1. Checked out the PR
  2. composer i
  3. created migration
  4. bumped info.xml
  5. started upgrade via webUI
  6. Migration runs through
  7. Profit!

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.

3 participants