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

Use Psr\Log from Composer #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ejegg
Copy link

@ejegg ejegg commented Aug 10, 2017

This PR removes a duplicated version of Psr\Log, which could have
caused applications to use a different version than expected. Instead,
we list psr/log as a composer dependency.

To facilitate unit tests, a phpunit-bootstrap file is added which
includes vendor/autoload.php, and a phpunit.xml file is added to point
to the bootstrap.

Running unit tests now requires 'composer install', but afterward you
simply run 'phpunit' in the root directory of the project.
'composer test' also works.

Phpunit is also added to the require-dev section of composer.json, as
the unit tests do depend on it.

Fixes #58

This commit removes a duplicated version of Psr\Log, which could have
caused applications to use a different version than expected. Instead,
we list psr/log as a composer dependency.

To facilitate unit tests, a phpunit-bootstrap file is added which
includes vendor/autoload.php, and a phpunit.xml file is added to point
to the bootstrap.

Running unit tests now requires 'composer install', but afterward you
simply run 'phpunit' in the root directory of the project.
'composer test' also works.

Phpunit is also added to the require-dev section of composer.json, as
the unit tests do depend on it.

Fixes amzn#58
@ejegg
Copy link
Author

ejegg commented Dec 15, 2017

Updated for 3.2.0, conflicts resolved. @bjguillot want to merge this? It looks pretty silly to have those duplicate Psr\Log files in this library.

@bjguillot
Copy link
Contributor

Thanks, @ejegg. I agree with you that the Psr\Log files should be removed since they duplicate code from another repository, but my concern is what to do about merchants that may be using the SDK without composer. There are some merchants that simply clone the files from GitHub directly into their production environment, and if were to remove the files, it could potentially complicate that method. It would also impact folks using the SDK in it's .phar form (perhaps bundling a second .phar for Psr\Log that could be separately downloaded)? If you have a suggestion on the best way to handle those aspects, that could speed along accepting the PR in the next SDK release. Thanks again for insisting on high standards.

@nlubisch
Copy link

nlubisch commented Dec 27, 2017

@bjguillot the Github Releases would be the right solution.
You can build the PHAR file and a ZIP file using Travis and push both as a Release -> Docs.
The build step pulls the dependency via composer and wraps it up in the PHAR/ZIP file.
This also prevents the PHAR file from being present in the repository itself and being pulled via Composer which in my case would be delivered to different merchants.

I'm also very interested in this, as i plan a major cleanup (PSR2, correct PHPDocs, and so on) for the current code base.
This helps me especially in working with the SDK for an upcoming migration from the old SDK.

@LeeSaferite
Copy link

What is the status of this?

I ask because I'm constantly getting error warnings in my IDE due to having two instances of Psr\Log\LoggerInterface in my project.

@nlubisch gave a valid answer for how to handle customers that don't use composer.

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.

4 participants