Skip to content

Conversation

@HLeithner
Copy link
Contributor

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

@laoneo
Copy link
Contributor

laoneo commented Jan 17, 2023

I think the tests should not be changed. Instead add new test functions which do test the new function.

@HLeithner
Copy link
Contributor Author

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.

@wilsonge
Copy link
Contributor

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 👍

@HLeithner
Copy link
Contributor Author

@nibra @Llewellynvdm would we like to merge this?

@Llewellynvdm
Copy link

Please re-base, and I will merge this.

@HLeithner
Copy link
Contributor Author

on 3?, I think adding it to a 2.x minor makes the b/c better for extensions developer

@wilsonge
Copy link
Contributor

I agree adding this into the 2.x minor makes more sense to help with migration paths

Copy link
Contributor

@wilsonge wilsonge left a 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)

@HLeithner
Copy link
Contributor Author

Created a new test based on loadAssoc using the deprecated syntax.

@Llewellynvdm
Copy link

Yes I was think 2 as well.

@Llewellynvdm Llewellynvdm merged commit c270ee1 into joomla-framework:2.0-dev Mar 21, 2023
@HLeithner HLeithner deleted the feature/createQuery branch July 4, 2023 14:27
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.

5 participants