Extract type cast behaviour

[zluiten]

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]

Any idea what's needed to get this in?

[weierophinney]

@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]

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

[weierophinney]

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.

[weierophinney]

I love that you've extracted this, and I think it will add a lot of flexibility!

I've provided some notes on improvements you should make before we merge. Please bump the minimum supported PHP version to 7.2 as part of your changes (update composer.json, and remove job configuration from .travis.yml), and add typehints whereever possible on new signatures only.

Finally, I didn't note it, but your tests should import classes used such as stdClass and DateTime.

[zluiten]

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

[zluiten]

@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]

Hey @weierophinney, can you have another look?

[weierophinney]

Hi, @Netiul !

I merged another PR earlier today that prepares the package for PHP 8... and solves the testing issues you were observing as well (mainly by updating a few key dependencies). Since I merged that, I rebased for you... and then discovered issues in that previous patch's phpcs updates, which meant I was fixing phpcs issues... anyways, it looks good; you incorporated all the feedback I had, and we now have running tests.

The only last think I need from you: could you write-up an example for the README.md file demonstrating substituting an alternative type caster implementation? (Or is that even necessary?)

[zluiten]

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