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

Update InstallSchema.php #9134

Closed
wants to merge 1 commit into from
Closed

Conversation

markoweb
Copy link

@markoweb markoweb commented Apr 5, 2017

Description

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title
  2. ...

Manual testing scenarios

  1. ...
  2. ...

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Apr 5, 2017

CLA assistant check
All committers have signed the CLA.

@adragus-inviqa
Copy link
Contributor

Yep, keep using those arrays for important stuff, people. You just love typing, don't you?! 👍

(You're not the target here, @markoweb.)

@orlangur
Copy link
Contributor

orlangur commented Apr 5, 2017

@adragus-inviqa, +1, some

$table->addColumn('first_name', 'string')
            ->setLength('255')
            ->setNotnull(true);

would be way better here ;)

Is there some community module integrating Doctrine into Magento 2 maybe?

@adragus-inviqa
Copy link
Contributor

Yeah, well, remember this? That didn't go too well, did it?

@okorshenko
Copy link
Contributor

@markoweb thank you for your contribution. Unfortunately, we can not accept this PR because we are not allowing changes in existing setup/upgrade scripts.
If you would like to fix this issue, please, bump module setup version and create upgrade script for it. In this case, everyone will be updated. But if we will update existing script, only new installations will get this update.

Please, submit new pull request once ready.

Thank you.

@okorshenko okorshenko closed this Apr 5, 2017
@okorshenko okorshenko self-assigned this Apr 5, 2017
@okorshenko okorshenko added this to the April 2017 milestone Apr 5, 2017
@orlangur
Copy link
Contributor

orlangur commented Apr 5, 2017

because we are not allowing changes in existing setup/upgrade scripts

Are you sure about that? Because I saw some changes by core team members where both setup and upgrade scripts were changed.

I do agree that changing only setup is not acceptable, but want to understand whether in this PR just upgrade part was missing or also change to setup was incorrect.

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

Successfully merging this pull request may close these issues.

5 participants