Skip to content
This repository has been archived by the owner on Apr 22, 2019. It is now read-only.

Fixed BP constant issue in registration.php #102

Merged
merged 2 commits into from
Jan 31, 2018
Merged

Fixed BP constant issue in registration.php #102

merged 2 commits into from
Jan 31, 2018

Conversation

makao
Copy link
Contributor

@makao makao commented Jan 3, 2018

This PR fixes issue when running CLI tools like PHPUnit, PHPMD or PHPCS. Composer is autoloading registration.php, but app/autoload.php isn't loaded there so BP constant is not available.

To reproduce try to run

php bin/magento dev:tests:run unit

It will throw an error

PHP Notice:  Use of undefined constant BP - assumed 'BP' in .../vendor/classyllama/module-avatax/registration.php on line 12

@erikhansen
Copy link
Contributor

erikhansen commented Jan 4, 2018

@makao I can't merge your PR as it stands, as it will cause an issue. Take a look at this commit and you'll see that your PR actually is an exact revert of the changes you made: https://github.com/makao/ClassyLlama_AvaTax/commit/30403feadb57f9ff13fffa1e2076d4eed07343f1#diff-f9a20ab0f1b66092b9c7efa82de1f220

I actually ran into a similar issue in my fork of this repo. In this fork, I was working on getting automated tests running with Travis CI. This is how I dealt with this issue: https://github.com/erikhansen/ClassyLlama_AvaTax/blob/feature/travis-ci-setup/registration.php

If you update this PR include the contents of the registration.php file, we can merge this PR.

@makao
Copy link
Contributor Author

makao commented Jan 4, 2018

I am not sure what do you mean by it will cause an issue. I've tried to compile DI and everything worked. However I've updated with check if BP is defined.

@erikhansen
Copy link
Contributor

@rsisco I'm quite certain this PR will work without causing any issues, but can you test locally and include these changes in your next release?

@molsm
Copy link

molsm commented Jan 29, 2018

Hello @makao and @erikhansen

Any updates on this? We are getting this issue too. Would be super cool to get this fix as fast as possible.

Best regards,

@makao
Copy link
Contributor Author

makao commented Jan 29, 2018

Still waiting, I've fixed it locally for development.

@rsisco rsisco merged commit f7a75ee into classyllama:develop Jan 31, 2018
@rsisco
Copy link

rsisco commented Jan 31, 2018

@makao - This has been included in the latest release - 1.3.1. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants