search

laminas-api-tools / api-tools-doctrine-querybuilder

Laminas API Tools Doctrine QueryBuilder module
https://api-tools.getlaminas.org/documentation
BSD 3-Clause "New" or "Revised" License
9 stars 17 forks source link

Extract type cast behaviour #5

Closed zluiten closed 3 years ago

zluiten commented 4 years ago

This is follow up on zfcampus/zf-doctrine-querybuilder#57

Q A
Bugfix no
BC Break no
New Feature yes
RFC no

Description

By extracting the type casting to its own class it can be easily replaced when needed. Preferring composition over inheritance etc.

It becomes better testable as well.

I kept BC by proxying to the TypeCaster and instantiating the TypeCaster in the AbstractFilter::getTypeCaster() method when not set in case FilterManager::filter() method is overridden.

zluiten commented 4 years ago

Any idea what's needed to get this in?

weierophinney commented 4 years ago

@netiul I've rebased against the new "1.9.x" branch; you'll need to pull from your own remote to get your local checkout up-to-date.

However, tests are failing.

I tried a number of different approaches to fix them, but it seems like there's something fundamentally broken in the testing suite or with the existing dependencies, and after a few hours trying to figure it all out, I need to leave that to others to resolve. Hopefully, if they get resolved, your tests will pass again, and we can merge and release.

zluiten commented 4 years ago

@weierophinney Thanks for the work you do and have done! About the tests, it looks like they passed on travis?

weierophinney commented 4 years ago

You're right! They hadn't been when I was testing before, but they clearly do now! The only issue is CS, which is not your problem to fix in this case (it's a line-length issue with our copyright lines). I'll get that fixed shortly.

zluiten commented 4 years ago

@weierophinney Apologies, I somehow completed missed you having reviewed this. Thanks though, I'll soon process your remarks.

zluiten commented 3 years ago

@weierophinney Thanks again for the review and apologies for the late reply. Incorporated your suggestions and meanwhile fixed a bug as noted here.

The Travis builds with lowest composer dependencies (--prefer-lowest) for 7.3 and 7.4 are (still) disabled. I'm not familiar with reason why those are disabled (I tested, builds do fail). I think it's out of scope of this PR to fix that.

I've also added support for immutable date doctrine field variants. That was actually the reason I wanted to extract the type casting behavior so I could replace the TypeCaster with one that supported the immutable date types.

zluiten commented 3 years ago

Hey @weierophinney, can you have another look?

zluiten commented 3 years ago

@weierophinney Thanks again. I've put something in the README.