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

Dependency Injection: fix custom loader on Windows #16364

Merged
merged 3 commits into from
Nov 19, 2020

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Nov 17, 2020

Context

  • Fixed Dependency Injection Compile command on Windows

Summary

This PR can be summarized in the following changelog entry:

  • Fixed Dependency Injection Compile command on Windows

Relevant technical choices:

Compile-DI: fix custom loader on Windows

Both the returned value from glob as well as the classmap generated by Composer contain file paths with a mix of forward and backward slashes when run on Windows.

This was breaking the compile-di command.

Previously it was silently broken - it wouldn't work, but without throwing an error.
Since the addition of the Schema_Blocks it would fail with an exception.

This commit fixes this by:

  1. Adding a normalize_slashes() method.
  2. Calling that method in the relevant places in the script to make sure that paths are normalized before they are compared based on their string value.

Custom_Loader::getClassFromClassMap(): efficiency fix

The Custom_Loader::getClassFromClassMap() would do a loop through all classes registered in the classmap via a foreach for each class requested (which could be many).

By flipping the array and using isset() instead, the functionality remains the same, but is infinitely faster.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

This should be tested on the three OS-es used by devs: Linux, MacOS and Windows.

  • On trunk, run composer install.
    On Linux and Mac this should be successful already, on Windows it will fail with an exception during the compile-di step.
  • After running the command, verify that the src/generated folder contains both the container.php and the container.php.meta file and that those are not empty.
  • Switch to this branch, rinse & repeat.
    If all is good, the command should now finish successfully on all three OSes.

Both the returned value from `glob` as well as the classmap generated by Composer contain file paths with a mix of forward and backward slashes when run on Windows.

This was breaking the `compile-di` command.

Previously it was _silently_ broken - it wouldn't work, but without throwing an error.
Since the addition of the `Schema_Blocks` it would fail with an exception.

This commit fixes this by:
1. Adding a `normalize_slashes()` method.
2. Calling that method in the relevant places in the script to make sure that paths are normalized before they are compared based on their string value.
The `Custom_Loader::getClassFromClassMap()` would do a loop through all classes registered in the classmap via a foreach for _each_ class requested (which could be many).

By flipping the array and using `isset()` instead, the functionality remains the same, but is infinitely faster.
@jrfnl jrfnl added yoast cs/qa changelog: other Needs to be included in the 'Other' category in the changelog labels Nov 17, 2020
@jrfnl jrfnl added this to the 15.5 milestone Nov 17, 2020
@afercia afercia self-assigned this Nov 19, 2020
@afercia
Copy link
Contributor

afercia commented Nov 19, 2020

CR 👍
Acceptance: tested on macOS and Windows 👍
Will ask a Linuxer to test.

Note: on Windows, the build fails for an unrelated, new, error on the task grunt shell:install-schema-blocks. Will check if it's a known issue or create a new one. (Note: happens also on trunk)

@afercia
Copy link
Contributor

afercia commented Nov 19, 2020

@enricobattocchi was so kind to test on his Linux machine: composer install works as expected.
Acceptance 👍 Merging!

@afercia afercia merged commit d0159d8 into trunk Nov 19, 2020
@afercia afercia deleted the JRF/fix-dependency-injection-compiling-on-windows branch November 19, 2020 14:11
@afercia afercia removed their assignment Nov 19, 2020
@increddibelly
Copy link
Contributor

works great on windows 👍

@igorschoester igorschoester added changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog and removed changelog: other Needs to be included in the 'Other' category in the changelog labels Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog yoast cs/qa
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants