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

API rescue master-branch PR: Shorten auto-generated table names #7621 #10454

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Aug 22, 2022

Rescues PR #7621 from the master branch.

Note that I have also added a commit on top of it which un-deprecates the warning for DataObject classes which are missing a table_name (added in #7620 as a companion to #7621) as I believe it promotes best practices and still protects against table names that may still be too long.

Parent issue

flamerohr and others added 3 commits August 22, 2022 17:44
Fix add check for legacy long table name and rename to short version
I think it's worth keeping this message around to encourage best
practices.
@GuySartorelli GuySartorelli mentioned this pull request Aug 22, 2022
5 tasks
@GuySartorelli
Copy link
Member Author

@emteknetnz You haven't asked me to change anything for this PR but also haven't merged it - I don't want to read into potential implications so can you please specify whether you want some changes made to this PR, and if so, what those changes are?

@emteknetnz emteknetnz merged commit 37ff4ee into silverstripe:5 Aug 31, 2022
@emteknetnz emteknetnz deleted the pulls/5/rescue-master-table-name branch August 31, 2022 21:12
@emteknetnz
Copy link
Member

This has caused a regression

1) SilverStripe\i18n\Tests\i18nTest::testGetExistingTranslations LogicException: Multiple classes ("SilverStripe\AssetAdmin\Tests\GraphQL\UnpublishFileMutationCreatorTest\FileOwner", "SilverStripe\AssetAdmin\Tests\Forms\FileFormBuilderTest\FileOwner") map to the same table: "SilverStripe_FileOwner" in /var/www/vendor/silverstripe/framework/src/ORM/DataObjectSchema.php:299

I tried reverting the change on my local and the issue went away

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Sep 1, 2022

@emteknetnz We could set explicit table_name config for each of those classes, but honestly this change that caused the regression is probably going to cause more pain than it's worth. I'm in favour of just reverting it. Do you agree?

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

Successfully merging this pull request may close these issues.

4 participants