-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add composer autoload #95
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good but we'll need to do a few more things:
- We have a GitHub Action that deploys plugin assets and readme changes. That will need to be updated to run
composer install
as well - We don't currently deploy the
vendor
directory so we'll need to update that or things won't work in the final release. We have both a.distignore
and.gitattributes
files that will need updated
@dkotter I've made the requested changes.
Note: I've updated the translators comment to match an example I found in WP Core. |
Regression / Smoke Test Report ✅Tested with the Testing Environment
|
@vikrampm1 @MaxwellGarceau E2E tests are failing here. Not sure if that is specific to the changes made here or something else but in getting merge conflicts cleaned up here to get this merged in for release, I think we should get those fixed up before we proceed, otherwise we'll need to fix those anyway on that release PR. |
@dkotter got it. @iamdharmesh can you able to assist with the above as @MaxwellGarceau is not resourced on this project anymore? |
This reverts commit 82116e0.
@dkotter E2E tests were failing because we are using the build release action from the develop branch, and it doesn't have the composer installation step. Due to an early return in the autoload file check, the plugin functionality was not getting enabled. I temporarily pointed the build release action to this PR, and it started working as expected. However, we recently enabled 2FA on the OSP account, and because of this, the connection tests and tests requiring connections are failing. I tried running the tests locally with a different account, and they worked well. So, it seems we are good to proceed here. cc: @vikrampm1 |
Description of the Change
Added autoloading to have an easy way to modularize new code and refactor existing code in the future.
❗I modified the Github action workflow files to install composer dependencies in order to ensure autoload was being generated on every build. I wanted to flag this in case I missed a required build process update.
Also, if there's a good place to add documentation for in progress pattern updates I'd be happy to add switching to autoload in new work and any refactors.
Closes #96
How to test the Change
Changelog Entry
Credits
Props @username, @username2, ...
Checklist: