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.

Sign up for GitHub

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

Jump to bottom

Deprecate omitting the alias in QueryBuilder::update() and delete() #9765

Closed
wants to merge 13 commits into from
Closed

Deprecate omitting the alias in QueryBuilder::update() and delete() #9765

wants to merge 13 commits into from

Conversation

Hanmac
Copy link
Contributor

@Hanmac Hanmac commented May 17, 2022

Closes #9733

Should the Trigger warning has the link to the Issue or to the PR?

@Hanmac @derrabus
Update lib/Doctrine/ORM/QueryBuilder.php
0023a69 
Co-authored-by: Alexander M. Turek <me@derrabus.de>
@Hanmac @derrabus
Update lib/Doctrine/ORM/QueryBuilder.php
1380a33 
Co-authored-by: Alexander M. Turek <me@derrabus.de>
@derrabus
Copy link
Member

Should the Trigger warning has the link to the Issue or to the PR?

The issue is just fine. Can you please add a small test for each method that checks if the exception is being raised?

@derrabus
Copy link
Member

Also, please run ./vendor/bin/phpcbf and commit the result.

derrabus
derrabus reviewed May 17, 2022
View reviewed changes
UPGRADE.md Outdated Show resolved Hide resolved
UPGRADE.md Outdated Show resolved Hide resolved
UPGRADE.md Outdated Show resolved Hide resolved
@Hanmac
Copy link
Contributor Author

Hanmac commented May 17, 2022

Also, please run ./vendor/bin/phpcbf and commit the result.

i'm only edited in github, i will try it later

any special php version under which i should do that?

Should the Trigger warning has the link to the Issue or to the PR?

The issue is just fine. Can you please add a small test for each method that checks if the exception is being raised?

currently there isn't any exception but trigger warning, but i will check on how such test would be done

Hanmac and others added 3 commits May 17, 2022 17:05
@Hanmac @derrabus
Update UPGRADE.md
d7973d0 
Co-authored-by: Alexander M. Turek <me@derrabus.de>
@Hanmac @derrabus
Update UPGRADE.md
9f71f90 
Co-authored-by: Alexander M. Turek <me@derrabus.de>
@Hanmac @derrabus
Update UPGRADE.md
f999f5e 
Co-authored-by: Alexander M. Turek <me@derrabus.de>
@Hanmac
Copy link
Contributor Author

Hanmac commented May 17, 2022

i will update the method description from update and delete later too so it is more clear that the two wanted use cased are:

  • with no parameters
  • with two parameters

@derrabus
Copy link
Member

currently there isn't any exception but trigger warning, but i will check on how such test would be done

There is a VerifyDeprecations trait that gives you some tooling for that. Just look into other tests that use this trait.

@derrabus derrabus added this to the 2.13.0 milestone May 19, 2022
@derrabus
Copy link
Member

The failing tests look related. Did you execute the tests locally?

derrabus
derrabus reviewed May 30, 2022
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for your PR.

The PR is still flagged as draft. Is there anything left to do from your side?

UPGRADE.md Outdated Show resolved Hide resolved
@derrabus derrabus changed the title add Deprecation for future Exceptions in QueryBuilder Deprecate omitting the alias in QueryBuilder::update() and delete() May 30, 2022
Hanmac and others added 2 commits May 30, 2022 20:51
@Hanmac @derrabus
Update UPGRADE.md
7b54ee2 
Co-authored-by: Alexander M. Turek <me@derrabus.de>
@Hanmac
Merge branch '2.13.x' into Hanmac-patch-1
62e4a39
@Hanmac
Copy link
Contributor Author

Hanmac commented May 30, 2022

Looks good to me. Thank you for your PR.

The PR is still flagged as draft. Is there anything left to do from your side?

i was thinking about adding the info into the method description telling the user that they should only do either zero or two arguments. But i might not be good with the wording :p

does the method description needs extra info that it might raise a deprecation or later an exception in the future?

@derrabus
Copy link
Member

Looks good to me. Thank you for your PR.
The PR is still flagged as draft. Is there anything left to do from your side?

i was thinking about adding the info into the method description telling the user that they should only do either zero or two arguments.

Feel free to do so, but I don't think it's necessary. Write the documentation that would've helped you. Don't worry too much about the wording.

does the method description needs extra info that it might raise a deprecation or later an exception in the future?

No. I don't think we need to document the deprecation explicitly here. If you add extra documentation, just say that either none or both parameters have to be passed.

@derrabus derrabus marked this pull request as ready for review June 17, 2022 08:27
derrabus added a commit that referenced this pull request Jun 17, 2022
@Hanmac @derrabus
Deprecate omitting the alias in QueryBuilder (#9765)
b931a59 
… methods update() and delete()

Co-authored-by: Alexander M. Turek <me@derrabus.de>
@derrabus
Copy link
Member

Closed via b931a59. Thanks @Hanmac!

@derrabus derrabus closed this Jun 17, 2022
derrabus added a commit to derrabus/orm that referenced this pull request Jun 17, 2022
@derrabus
Merge branch '2.13.x' into 3.0.x
318e6ec 
* 2.13.x:
  Deprecate omitting the alias in QueryBuilder (doctrine#9765)
  Run tests on PHP 8.2 (doctrine#9840)
  PHPStan 1.7.13 (doctrine#9844)
  Flip conditional extension of legacy AnnotationDriver class (doctrine#9843)
  PHP CodeSniffer 3.7 (doctrine#9842)
  Make Reflection available to ConvertMappingCommand (doctrine#9619)
  Add missing property declaration
  Use proper API for introspection of tables
@derrabus derrabus mentioned this pull request Aug 3, 2022
Merged
@Hanmac Hanmac deleted the Hanmac-patch-1 branch July 6, 2023 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus left review comments

Assignees
No one assigned
Labels
Deprecation
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Hanmac @derrabus