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

Feature/updated autoloading #236

Open
wants to merge 18 commits into
base: trunk
Choose a base branch
from
Open

Conversation

darylldoyle
Copy link
Collaborator

@darylldoyle darylldoyle commented Aug 13, 2024

Description of the Change

This PR updates the class autoloader from using https://gitlab.com/hpierce1102/ClassFinder/ to using https://github.com/spatie/php-structure-discoverer and adds unit tests to ensure it's working as expected.

Spatie are a well known company, who specialise in Open Source packages. This means the package is updated more often and bugs will be fixed more quickly. For example, the last commit in hpierce1102/classfinder was June 18, 2023, whereas the last commit in spatie/php-structure-discoverer was today.

There are a few other benefits of the Spatie package:

  • It doesn't rely on the composer.json to load files, meaning we no longer have to deploy that.
  • There's no hidden logic; we can see exactly how the classes are being loaded now and what constraints are being put on them.
  • We can conditionally cache the class loader, giving it a considerable performance boost on production/staging.

Old Class Loader
Screenshot 2024-08-13 at 23 22 45

New Class Loader
Screenshot 2024-08-13 at 23 21 50

Additionally, during the refactor, I eliminated all the hardcoded strings and awkward configurations needed to get the classloader working. It should now be plug-and-play.

One downside of this change is that it only supports PHP 8.1 upwards, though since every version pre-8.1 is End of Life, I think that's a fair tradeoff, https://www.php.net/supported-versions.php.

How to test the Change

Ensure all classes are still loading and test suite is passing.

Changelog Entry

  • Changed - Class loading swapped out for a newer, more performant library.

Credits

Props @darylldoyle

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@darylldoyle darylldoyle marked this pull request as ready for review August 14, 2024 11:11
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an updated version of the default install-wp-tests.sh, it has a few benefits:

  • It has a help command, which ensures engineers that are new to PHPUnit have a way to understand the params of the script
  • When run from a Local WP shell, it automatically picks up and uses the MySQL socket.
    It has a sensible set of defaults that will work for 99% of 10up builds, meaning you can run the script without passing any parameters.
  • It resolves an issue where the install script thinks it's already installed (because the /tmp directory exists, but the DB isn't installed.

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.

1 participant