-
Notifications
You must be signed in to change notification settings - Fork 392
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
Remove composer deps #305
Remove composer deps #305
Conversation
I think for v2 we should remove the composer deps from the master branch, and tagged version should be from that branch. THis means installing the plugin via composer won't cause additoinal copies of the AWS SDK to exist. We can still have a build branch, and upload artifacts to the release page I believe, for people who want to use the plugin without composer.
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.
Linting failed (1032 errors, 81 warnings).
1113 notices occurred in your codebase, but none on files/lines included in this PR.
@joehoyle did you remove the hacks to the AWS SDK? I thought we still had some, which necessitated not using Composer/etc here. |
@rmccue that was a long time ago, those haven't been needed for a while |
@@ -18,7 +18,7 @@ function s3_uploads_init() { | |||
// Ensure the AWS SDK can be loaded. | |||
if ( ! class_exists( '\\Aws\\S3\\S3Client' ) ) { |
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.
I feel like we should maybe change this to instead be if file_exists vendor/autoload
instead now?
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.
Hmm I think it's better the way it already was - including the autoload file will cause issues is S3Client
is already loaded via another autoload (I think).
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.
this is causing issues as it thows a fatal error stating No such file or directory in /wp-content/plugins/S3-Uploads-2.2.1/vendor/autoload.php
.
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.
@tylerlindell You need to use Composer to install the dependencies when updating. (Also, please file a new issue rather than commenting on old PRs, otherwise we can easily miss it!)
I think for v2 we should remove the composer deps from the master branch, and tagged version should be from that branch. THis means installing the plugin via composer won't cause additoinal copies of the AWS SDK to exist.
We can still have a build branch, and upload artifacts to the release page I believe, for people who want to use the plugin without composer.