-
-
Notifications
You must be signed in to change notification settings - Fork 36
Replace getQuery(true) with createQuery(), deprecate $new parameter #274
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
Replace getQuery(true) with createQuery(), deprecate $new parameter #274
Conversation
|
I think the tests should not be changed. Instead add new test functions which do test the new function. |
|
The functionality it self is tested at https://github.com/joomla-framework/database/blob/2.0-dev/Tests/DatabaseFactoryTest.php#L261-L272 if we keep getQuery(true) in the test they will issue deprecation warnings. Maybe a new test for ->getQuery(true) which expects an deprecation warning make sense. In v3 the interface should also get the new method. |
|
I don't think having a boolean param is inherently bad. But in this case given it's the majority use case it definitely makes sense to separate this so 👍 |
|
@nibra @Llewellynvdm would we like to merge this? |
|
Please re-base, and I will merge this. |
|
on 3?, I think adding it to a 2.x minor makes the b/c better for extensions developer |
|
I agree adding this into the 2.x minor makes more sense to help with migration paths |
wilsonge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave one test in using getQuery(true) to test the legacy behavior still works (happy with moving the majority of test to the new style)
|
Created a new test based on loadAssoc using the deprecated syntax. |
|
Yes I was think 2 as well. |
Since using a boolean parameter is a bad pattern and setting the parameter to true to get a new object is miss understandable and makes it hard for new comers to use the API.
The pull requests create a new method
createQuery()which always returns a new databasequery. It also depreacte and mark the true parameter for removal in version 4.0