joomla-framework / database

Joomla Framework Database Package
GNU General Public License v2.0
28 stars 35 forks source link

Replace getQuery(true) with createQuery(), deprecate $new parameter #274

Closed HLeithner closed 1 year ago

HLeithner commented 1 year ago

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 commented 1 year ago

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

HLeithner commented 1 year ago

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 commented 1 year ago

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 :+1:

HLeithner commented 1 year ago

@nibra @Llewellynvdm would we like to merge this?

Llewellynvdm commented 1 year ago

Please re-base, and I will merge this.

HLeithner commented 1 year ago

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

wilsonge commented 1 year ago

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

HLeithner commented 1 year ago

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

Llewellynvdm commented 1 year ago

Yes I was think 2 as well.