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.
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