8000 Support mysql8 by staabm · Pull Request #3253 · redaxo/redaxo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support mysql8 #3253

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
Merged
merged 30 commits into from
Feb 2, 2020
Merged

Support mysql8 #3253

merged 30 commits into from
Feb 2, 2020

Conversation

staabm
Copy link
Member
@staabm staabm commented Jan 30, 2020 •
edited
Loading

Inspired by doctrine/dbal#3372

Closes #2351

@staabm
Copy link
Member Author
staabm commented Jan 30, 2020

Interessant.. hier gibts test errors.. in #3254 mit mysql 8 ist alles grün

@staabm
Copy link
Member Author
staabm commented Feb 1, 2020 •
edited
Loading

@gharlan @bloep ich denke es wäre super wenn wie die paar tests für mysql8 noch grün bekommen für 5.9... vllt komm ich morgen dazu.. falls jemand von euch dazu kommt, gerne hier weitermachen

Ich vermute dass redaxo auf mysql8 aktuell nicht wirklich funktionieren kann, so wie die testfehler ausschauen

@staabm staabm added this to the REDAXO 5.9 milestone Feb 1, 2020
@staabm staabm added Enhancement Improvements for existing features Core REDAXO Core related things labels Feb 1, 2020
@staabm
Copy link
Member Author
staabm commented Feb 1, 2020 •
edited
Loading 
There were 3 errors:
388
3891) rex_sql_table_test::testAddForeignKey
390Undefined index: constraint_name
391
392redaxo/src/core/lib/sql/table.php:138
393redaxo/src/core/lib/sql/table.php:162
394redaxo/src/core/lib/base/instance_pool_trait.php:70
395redaxo/src/core/lib/sql/table.php:163
396redaxo/src/core/tests/sql/sql_table_test.php:442
397
3982) rex_sql_table_test::testEnsureForeignKey
399Undefined index: constraint_name
400
401redaxo/src/core/lib/sql/table.php:138
402redaxo/src/core/lib/sql/table.php:162
403redaxo/src/core/lib/base/instance_pool_trait.php:70
404redaxo/src/core/lib/sql/table.php:163
405redaxo/src/core/tests/sql/sql_table_test.php:472
406
4073) rex_sql_table_test::testRenameForeignKey
408Undefined index: constraint_name
409
410redaxo/src/core/lib/sql/table.php:138
411redaxo/src/core/lib/sql/table.php:162
412redaxo/src/core/lib/base/instance_pool_trait.php:70
413redaxo/src/core/lib/sql/table.php:163
414redaxo/src/core/tests/sql/sql_table_test.php:493
415
416--
417
418There were 2 failures:
419
4201) rex_sql_table_test::testCreate
421Failed asserting that two strings are identical.
422--- Expected
423+++ Actual
424@@ @@
425-'int(11)'
426+'int'
427
428redaxo/src/core/tests/sql/sql_table_test.php:71
429
4302) rex_sql_table_test::testAlter
431Failed asserting that two strings are identical.
432--- Expected
433+++ Actual
434@@ @@
435-'int(10) unsigned'
436+'int unsigned'
437
438redaxo/src/core/tests/sql/sql_table_test.php:537
439
440ERRORS!
441Tests: 441, Assertions: 1052, Errors: 3, Failures: 2, Skipped: 1.

https://dev.mysql.com/doc/refman/8.0/en/information-schema.html

@staabm
Copy link
Member Author
staabm commented Feb 1, 2020

hmm

1) rex_sql_table_test::testCreate

Failed asserting that two strings are identical.

--- Expected

+++ Actual

@@ @@

-'int(11)'

+'int'

redaxo/src/core/tests/sql/sql_table_test.php:72

2) rex_sql_table_test::testAlter

Failed asserting that two strings are identical.

--- Expected

+++ Actual

@@ @@

-'int(10) unsigned'

+'int unsigned'

mysql8 scheint hier die länge der spalten nicht drinnen zu haben.. lt docs sollten die dort aber drinne sein..
https://dev.mysql.com/doc/refman/8.0/en/show-columns.html

@staabm staabm marked this pull request as ready for review February 1, 2020 13:42
@staabm staabm requested a review from gharlan February 1, 2020 13:47
staabm
staabm commented Feb 1, 2020
View reviewed changes
staabm
staabm commented Feb 1, 2020
View reviewed changes
redaxo/src/core/lib/sql/column.php
@@ -104,7 +104,7 @@ public function setType($type)
}

/**
* @return string
* @return string The column type, including its size, e.g. int(10) or varchar(255)
Copy link
Member Author

Choose a reason for hiding this comment

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

das mit dem "inlcuding size" scheint für mysql8 noch nicht zu passen.. aus BC gründen müsste man es aber versionsübergreifend hier gleich haben... keine ahnung woran es da hängt

Copy link
Member
@gharlan gharlan Feb 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Meine erste Vermutung war, dass es vllt nur bei der Max Size (also 11 für signed und 10 für unsigned) so ist.
Aber ist auch bei kleineren ints so, habe dafür noch einen Test ergänzt.

Konnte zu der Problematik leider auch noch nichts weiter finden.

Die Doku zu mysql 8 behauptet auch, es würde "int(11)" zurückkommen:

https://dev.mysql.com/doc/refman/8.0/en/show-columns.html

Copy link
Member

Choose a reason for hiding this comment

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

ich schau mir das nachher mal an und such mal ein bissl.

Copy link
Member

Choose a reason for hiding this comment

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

Ggf. hängt es hiermit zusammen?
https://dba.stackexchange.com/a/184546

Copy link
Member

Choose a reason for hiding this comment

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

Ich denke, ich habe endlich den Grund gefunden:
https://dev.mysql.com/doc/refman/8.0/en/numeric-type-attributes.html

MySQL supports an extension for optionally specifying the display width of integer data types in parentheses following the base keyword for the type. For example, INT(4) specifies an INT with a display width of four digits. This optional display width may be used by applications to display integer values having a width less than the width specified for the column by left-padding them with spaces. (That is, this width is present in the metadata returned with result sets. Whether it is used is up to the application.)

und

As of MySQL 8.0.17, the ZEROFILL attribute is deprecated for numeric data types, as is the display width attribute for integer data types. Support for ZEROFILL and display widths for integer data types will be removed in a future MySQL version. Consider using an alternative means of producing the effect of these attributes.

Ich überlege mir was, wie wir damit umgehen.

Copy link
Member

Choose a reason for hiding this comment

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

Vorschlag ist nun drin.
So würde ich zumindest nun für die 5.9 verfahren.
Für zukünftige Versionen könnte man dann eher schauen, mehr zu int und int unsigned zu kommen. Also statt wie jetzt ein Workaround für MySQL 8, dann ein Workaround eher für die anderen Versionen.

gharlan
gharlan reviewed Feb 1, 2020
View reviewed changes
staabm and others added 2 commits February 1, 2020 15:33
@staabm
Merge branch 'master' into staabm-patch-2
< 8000 input type="hidden" name="id" value="MDY6Q29tbWl0NzU2NDg1OjJiMjc5ZDE0YmNiODdiYjBiNzY0OTI1MmM3MjFkNjMxMWJiY2ZhNWE=" autocomplete="off" data-targets="batch-deferred-content.inputs" />
2b279d1
@gharlan
INFORMATION_SCHEMA spalten groß geschrieben
9c6f7f5
@gharlan gharlan changed the title Build travis with mysql8 Support mysql8 Feb 1, 2020
@staabm
Copy link
Member Author
staabm commented Feb 2, 2020

Noch als idee eingeworfen: vllt kann man mal bei phpmyadmin/adminer o.ä. Schauen wie das dort gehandhabt wird

@staabm
Copy link
Member Author
staabm commented Feb 2, 2020

https://github.com/phpmyadmin/phpmyadmin/blob/94888eec39dc066a83082b9145046c948bcf743c/libraries/classes/Plugins/Export/ExportTexytext.php#L587

https://github.com/phpmyadmin/phpmyadmin/blob/a972a00c9bccd30b1be8326538dcef74783c6dec/libraries/classes/Util.php#L1454

gharlan
gharlan approved these changes Feb 2, 2020
View reviewed changes
Copy link
Member
@gharlan gharlan left a comment

Choose a reason for hiding this comment

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

@schuer hat den PR über Docker mit MySQL 8.0.0 und der aktuellen 8.0.19 kurz angetestet und funktioniert soweit!

staabm
staabm commented Feb 2, 2020
View reviewed changes
redaxo/src/core/lib/sql/table.php
@@ -85,9 +85,19 @@ private function __construct($name, int $db = 1)
}

foreach ($columns as $column) {
$type = $column['type'];

// Since MySQL 8.0.17 the display width for integer columns is deprecated.
Copy link
Member Author

Choose a reason for hiding this comment

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

Gibts dazu nen link von mysql selbst?

Copy link
Member

Choose a reason for hiding this comment

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

Siehe #3253 (comment)

staabm
staabm commented Feb 2, 2020
View reviewed changes
Copy link
Member Author
@staabm staabm left a comment

Choose a reason for hiding this comment

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

Danke fürs abschließen

staabm
staabm commented Feb 2, 2020
View reviewed changes
@staabm staabm added the automerge Automatisch PR rebasen und mergen label Feb 2, 2020
@kodiakhq kodiakhq bot merged commit b703d5c into master Feb 2, 2020
@kodiakhq kodiakhq bot deleted the staabm-patch-2 branch February 2, 2020 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gharlan gharlan gharlan approved these changes

@bloep bloep bloep left review comments

Assignees
No one assigned
Labels
automerge Automatisch PR rebasen und mergen Core REDAXO Core related things Enhancement Improvements for existing features
Milestone
REDAXO 5.9
Development

Successfully merging this pull request may close these issues.

Mysql8 travis build
3 participants
@staabm @gharlan @bloep
0