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

Add __constructor 3rd parameter's default value #16

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

allanpaiste
Copy link

Encountered an error when upgrading to Magento EE 2.1.6 relates to the fact that under certain considtions Modman parser will have no contructor arguments defined (which used to be expected in the original module that magento team has forked). Due to that module stating that it's a replacement for magento-hackathon module, I would expect the __construct setup to remain intact for it to really serve as areplacement. Therefore, a default value was introduced for the third argument.

- Installing colinmollenhour/cache-backend-file (1.4)
  Loading from cache
[ErrorException]
Missing argument 3 for MagentoHackathon\Composer\Magento\ModmanParser::__construct(), called in phar:///usr/local/bin/composer/src/Composer/Plugi
n/PluginManager.php(195) : eval()'d code on line 318 and defined

Allan Paiste added 4 commits April 21, 2017 14:44
Changed the readme to refer to the fork-chain that has been happening in reality
add a default value to __construct's 3rd argument as two first have a default and having 3rd without one causes crashes with some older code.
Rollback on readme
@magento-cicd2
Copy link

magento-cicd2 commented Apr 21, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@piotrekkaminski
Copy link

Approved

@allanpaiste
Copy link
Author

Cool! I don't have the authority to merge this though. But it's cool that it finally got approved :)

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