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

fix for broken alter-table statements when default column values cont… #1566

Closed

Conversation

hevengo
Copy link

@hevengo hevengo commented Jun 22, 2020

The problem

When a column defaultValue contains a semicolon (;) the generated alter-table statement is broken.

<column name="attachement_allowed_extensions" phpName="AttachementAllowedExtensions" 
type="VARCHAR" size="250" required="true" defaultValue="pdf;jpg;png;doc;docx;xls;xlsx;txt"/>

the reason for that can be found in file /src/Propel/Generator/Platform/DefaultPlatform.php line 818ff

if ($columnChanges) {
    //merge column changes into one command. This is more compatible especially with PK constraints.

    $changes = explode(';', $columnChanges);

the already generated sql string consists of multiple alter table statements and shall be split into single alter table statements to merge. If the generated statement

ALTER TABLE `newsletter_sets`  CHANGE `attachement_allowed_extensions` `attachement_allowed_extensions` VARCHAR(250) DEFAULT 'pdf;jpg;png;doc;docx;xls;xlsx;txt' NOT NULL;

contains multiple semicolons so the splitting goes wrong.

The solution

propel already contains a sql statement parser that can be used as a drop-in replacement here.

$sqlParser = new SqlParser();
$sqlParser->setSQL($columnChanges);
$changes = $sqlParser->explodeIntoStatements();

@dereuromark
Copy link
Contributor

We now have master almost green ( https://travis-ci.org/github/propelorm/Propel2 )
Please try to rebase yours against that one, that will make it easier to verify your changes.
Do you happen to have a test case along with your change request?

@hevengo
Copy link
Author

hevengo commented Jun 22, 2020

i rebased it against the current master - i will try to build a testcase

@hevengo
Copy link
Author

hevengo commented Jun 22, 2020

i see that some testCases are failing now due to some different newlines. What would you recommend me to do?

1) Propel\Tests\Generator\Platform\OraclePlatformMigrationTest::testGetModifyDatabaseDDL with data set #0 (Propel\Generator\Model\Diff\DatabaseDiff Object (...))
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 \n
 CREATE SEQUENCE foo5_SEQ\n
     INCREMENT BY 1 START WITH 1 NOMAXVALUE NOCYCLE NOCACHE ORDER;\n
-\n
 ALTER TABLE foo2 RENAME COLUMN bar TO bar1;\n
 \n
 ALTER TABLE foo2\n
/home/travis/build/propelorm/Propel2/tests/Propel/Tests/Generator/Platform/OraclePlatformMigrationTest.php:73

https://travis-ci.org/github/propelorm/Propel2/jobs/700959913

@dereuromark
Copy link
Contributor

Before, if (!trim($change)) continue; took care of empty rows
You might want to check what your changed method returns here instead.

@dereuromark
Copy link
Contributor

ping @hevengo

@vol4onok vol4onok mentioned this pull request Mar 28, 2024
@gechetspr
Copy link
Contributor

Merged in #1997

@gechetspr gechetspr closed this Apr 1, 2024
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