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 Don't generate table alias for "from" statement that are not column names #11312

Merged

Conversation

maxime-rainville
Copy link
Contributor

Manual testing steps

This is updating a low level API that is widely use across tho codebase. There's not really any manual steps that could be taken ... aside from using the system.

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

public function subqueryProvider()
{
return [
'no-alias-string' => ['( SELECT DISTINCT "SQLSelectTest_DO"."ClassName" FROM "SQLSelectTest_DO") AS "FINAL"'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the only scenario that would have failed prior to this PR. The other ones are just there for the sake of completeness.


public function addFromProvider()
{
return [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of those would have pass previously, but addFrom didn't have explicit test coverage.

src/ORM/Queries/SQLConditionalExpression.php Outdated Show resolved Hide resolved
maxime-rainville added a commit to archiprocode/recipe-kitchen-sink that referenced this pull request Jul 26, 2024
@maxime-rainville
Copy link
Contributor Author

Got a sink build here with this PR: https://github.com/archiprocode/recipe-kitchen-sink/actions/runs/10103439804

The Lining failure is pre-existing.

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.

I'd love to have $alias as a second parameter, so aliases have to be explicitly added instead of the system guessing what it should be.
But that would have to be a major release change so obviously isn't appropriate for this PR.

Seems like a good solution. Just a couple small changes to make.

src/ORM/Queries/SQLConditionalExpression.php Outdated Show resolved Hide resolved
src/ORM/Queries/SQLConditionalExpression.php Outdated Show resolved Hide resolved
src/ORM/Queries/SQLConditionalExpression.php Outdated Show resolved Hide resolved
tests/php/ORM/SQLSelectTest.php Outdated Show resolved Hide resolved
Copy link
Contributor Author

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

I'll have implement the requested change tomorrow.

src/ORM/Queries/SQLConditionalExpression.php Outdated Show resolved Hide resolved
src/ORM/Queries/SQLConditionalExpression.php Outdated Show resolved Hide resolved
tests/php/ORM/SQLSelectTest.php Outdated Show resolved Hide resolved
@maxime-rainville maxime-rainville self-assigned this Jul 29, 2024
@maxime-rainville maxime-rainville removed their assignment Jul 29, 2024
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.

LGTM, thanks for doing this.

@GuySartorelli GuySartorelli merged commit 7c27e9d into silverstripe:5.2 Jul 29, 2024
15 checks passed
@maxime-rainville maxime-rainville deleted the pulls/5.2/sqlselect-from-alias branch July 29, 2024 21:57
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