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

BUG Use more robust logic to retrieve generated ID #148

Conversation

maxime-rainville
Copy link

@maxime-rainville maxime-rainville commented Jun 28, 2023

I not sure why, but occasionally the sequence column for an ID column won't match it's expected name. That came through when scaffolding a many_many_through relation for an eager loading test.

Following the recommendation from this Stack overflow answer, I've updated the logic to use a more robust approach.

Parent issue

@maxime-rainville
Copy link
Author

I'm not adding unit test here because I'm honestly not sure how to systematically replicate the issue.

@GuySartorelli
Copy link
Member

I'm not adding unit test here because I'm honestly not sure how to systematically replicate the issue.

Does this mean the CI failure is intermittent as well? If this is intermittent, I'm not really sure how to manually test this is working, then?

@maxime-rainville
Copy link
Author

I'm assuming that adding an item to many_many relations is covered somewhere in our unit tests. For some reason, the many_many in Steve's test always fails, but I'm not sure why.

I've targeted the fix for this to 3.0 because it seems like a bug that shouldn't require a new minor. However, the version of framework that is being run by the unit test doesn't include the new eager loading feature.

You could minimally accept that this is PR doesn't make things worst.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

pg_get_serial_sequence isn't documented anywhere on https://www.php.net which is a bit of a concern... it is mentioned here but I can't find out if that documentation refers to the same php plugin that the rest of the module relies on....
That said it's not failing in CI and this module doesn't declare a specific PHP module as a dependency anyway so that's probably fine.

Looks good to me on a "doesn't seem to make anything worse" basis. No idea if it fixes the CI failure 😅

@GuySartorelli GuySartorelli merged commit 4608fd7 into silverstripe:3.0 Jun 29, 2023
@GuySartorelli GuySartorelli deleted the pulls/3.0/better-get-generated-id-logic branch June 29, 2023 21:37
@maxime-rainville
Copy link
Author

pg_get_serial_sequence is a PostgreSQL function. There's no reason why it would be in the PHP doc. The same way that MySQL specifc feature won't be documented on the PHP site either.

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.

2 participants