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.

Sign up for GitHub

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

Jump to bottom

Fix doctrine/data-fixtures PR#334 #340

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Fix doctrine/data-fixtures PR#334 #340

merged 1 commit into from
Jan 17, 2020

Conversation

stephen-lewis
Copy link
Contributor

PR #334 broke existing code by adding quotes in the middle of table names that used schemas in SQLite - as reported in #338 .

I've reworked it into a cleaner function that mimics SchemaTool in doctrine/orm. Tests included and pass locally; I've also setup and run a simple test project locally using schema and non-schema entities without issue.

@greg0ire
Copy link
Member

greg0ire commented Jan 15, 2020 •
edited
Loading

@stephen-lewis can you please run this script from your fork? It should produce introduce for people who need to test.

@greg0ire greg0ire changed the base branch from master to 1.4.x January 15, 2020 20:52
@stephen-lewis
Copy link
Contributor Author

stephen-lewis commented Jan 15, 2020 •
edited
Loading

@stephen-lewis can you please run this script from your fork? It should produce introduce for people who need to test.

Sure; output below:

How can I test this?

composer config repositories.stephen-lewis vcs https://github.com/stephen-lewis/data-fixtures
composer require doctrine/data-fixtures "dev-bugfix/fix_errors_from_PR334 as 1.4.1"

--edit: fix shell command typo

@greg0ire
Copy link
Member

@jgxvx please use the instructions above to test this on your project and report back 🙏 You might have to use the no-api key, but I'm not 100% sure about that, please see a usage example here: composer/composer#5433

@greg0ire greg0ire added the Bug label Jan 15, 2020
@greg0ire
Copy link
Member

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/1.4.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

@stephen-lewis stephen-lewis mentioned this pull request Jan 15, 2020
Closed
@greg0ire greg0ire mentioned this pull request Jan 16, 2020
Closed
@jgxvx
Copy link

jgxvx commented Jan 17, 2020

@greg0ire @stephen-lewis Glad to confirm that the fix works!

@greg0ire
Copy link
Member

greg0ire commented Jan 17, 2020 •
edited
Loading

@stephen-lewis please improve your commit message according to the contributing guide, then I will merge

TL;DR say what was broken and how your changes improve the situation

@stephen-lewis
Fix ORMPurger generating table names with unnecessary quotes
355199c 
Change in 1.4.1 (doctrine#334) caused ORMPurger to generate quotes inside table
names with mocked schemas leaving them invalid (e.g.
test_schema__"group") becuase quoting strategy was ignored. Reverted
changes to ORMPurger::getTableName and added new fucntion to mimic
behaviour of SchemaTool in doctrine/orm when generating delete purge
SQL. Unit tests included to check for correct SQL generation for tables
both with and without schema. Fixes doctrine#338.
@stephen-lewis
Copy link
Contributor Author

@greg0ire commit message should now be updated 😄

greg0ire
greg0ire approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gorgeous commit message, thanks!

@greg0ire greg0ire merged commit 39e9777 into doctrine:1.4.x Jan 17, 2020
@greg0ire greg0ire added this to the 1.4.2 milestone Jan 17, 2020
@doctrinebot doctrinebot mentioned this pull request Jan 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
1.4.2
Development

Successfully merging this pull request may close these issues.
3 participants
@stephen-lewis @greg0ire @jgxvx