-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Prevent too long identifier names #10221
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
Conversation
|
Fails for the calendar: |
|
Yeah wanted to fail it and now rebasing on #10213 |
06b8f90 to
5a9d517
Compare
MorrisJobke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works now 👍 Also code makes sense
|
Can't this still do 💥 is people chose a to long prefix? |
Well, that's the case for any table, not only this resource booking ones. If someone selects a 30-char long prefix it will just go 💥 |
| } | ||
|
|
||
| public function ensureOracleIdentifierLengthLimit(Schema $schema) { | ||
| foreach ($schema->getTables() as $table) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1) Test\DB\MigrationsTest::testExecuteStepWithSchemaChange
Invalid argument supplied for foreach()
/drone/src/github.com/nextcloud/server/lib/private/DB/MigrationService.php:464
/drone/src/github.com/nextcloud/server/lib/private/DB/MigrationService.php:451
/drone/src/github.com/nextcloud/server/tests/lib/DB/MigrationsTest.php:120
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and also added unit tests for all the checks
Well if you use oracle you usually dont change such stuff anyway, because you know the limitations. The current code makes sure it works with a default prefix (3 chars). Anything else is not our business |
|
|
Right, default name on postgres is also not |
|
|
|
Postgres is so annoying, anyway pushed a fix |
|
|
I shall set it up locally for faster debugging this week. |
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
a349ee0 to
ef5074a
Compare
|
Fixed and tested locally with postgres, mysql and sqlite |
rullzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐘
Ref #10213