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

[3.x/4.x]: Emails::_createEmailQuery (and others) could be explicit about the orderBy #3263

Closed
msbit opened this issue Sep 2, 2023 · 3 comments · Fixed by #3267
Closed
Labels
bug commerce4 Issues related to Commerce v4

Comments

@msbit
Copy link
Contributor

msbit commented Sep 2, 2023

What happened?

A slightly curly one, but in my test suite (running on SQLite in memory, you don't see this on MySQL) calling \craft\commerce\services\Emails::getAllEmailsByOrderStatusId fails with the following:

yii\db\Exception: SQLSTATE[HY000]: General error: 1 ambiguous column name: name
Failed to prepare SQL: SELECT `emails`.`id`, `emails`.`name`, `emails`.`subject`, `emails`.`recipientType`, `emails`.`to`, `emails`.`bcc`, `emails`.`cc`, `emails`.`replyTo`, `emails`.`enabled`, `emails`.`templatePath`, `emails`.`plainTextTemplatePath`, `emails`.`uid`, `emails`.`pdfId`, `emails`.`language` FROM `commerce_emails` `emails` INNER JOIN `commerce_orderstatus_emails` `statusEmails` ON `emails`.`id` = `statusEmails`.`emailId` INNER JOIN `commerce_orderstatuses` `orderStatuses` ON `statusEmails`.`orderStatusId` = `orderStatuses`.`id` WHERE `orderStatuses`.`id`=:qp0 ORDER BY `name`

This boils down to calling:

->orderBy('name')

without specifying the table (like so):

->orderBy('emails.name')

at:

https://github.com/craftcms/commerce/blob/3.4.22.1/src/services/Emails.php#L913
https://github.com/craftcms/commerce/blob/4.2.11/src/services/Emails.php#L894

when used in scenarios where a join against a table which also has a name column (such as commerce_orderstatuses in this case).

It seems like it would be a good idea (and help out my specific case :D) to provide the table alias to the orderBy call here (and perhaps anywhere else like this).

Craft CMS version

3.8.9

Craft Commerce version

3.4.21

PHP version

No response

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

@msbit msbit added commerce4 Issues related to Commerce v4 bug labels Sep 2, 2023
msbit added a commit to msbit/commerce that referenced this issue Sep 7, 2023
Add the table alias to the `orderBy` to avoid driver specific ambiguity in
the generated SQL when joining against another table that _also_ has a `name`
column.

This is in line with `States::_createStatesQuery`, and should fix craftcms#3263.
msbit added a commit to msbit/commerce that referenced this issue Sep 7, 2023
Add the table alias to the `orderBy` to avoid driver specific ambiguity in
the generated SQL when joining against another table that _also_ has a `name`
column.

This is in line with `States::_createStatesQuery`, and should fix craftcms#3263.
@msbit
Copy link
Contributor Author

msbit commented Sep 7, 2023

Thanks @lukeholder for the quick merge! I took a guess at your dev process; unsure if there also needs to be a PR based off the 4.3 branch?

@msbit
Copy link
Contributor Author

msbit commented Oct 8, 2023

Hey @lukeholder, are you planning on including the PR for this in a 3.x release?

@nfourtythree
Copy link
Contributor

Hi @msbit

Thank you for reporting this to us and adding the PRs to solve it!

Just wanted to update you to let you know that the fix made it into 4.3.0 and (the just tagged) 3.4.23 versions of Commerce.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug commerce4 Issues related to Commerce v4
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants