Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mongodb date_immutable filter support #3940

Merged

Conversation

CheapHasz
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Test pass? kinda, (the tests I added pass, but I wasn't able to start the full mongob_phpunit unit test suite, it was failing on a Prophecy dependency. running only the tests/Bridge/Doctrine/MongoDbOdm/PropertyInfo/DoctrineExtractorTest.php does pass though)

DateFilter didn't work on date_immutable ODM/Field

@alanpoulain
Copy link
Member

alanpoulain commented Jan 4, 2021

Hello. Thanks for the PR. You should check the CS and PHPStan.
I don't remember why it has not been done before. Is it a recent addition in MongoDB ODM?

@CheapHasz CheapHasz force-pushed the mongodb_date_immutable_filter branch from ac0befd to c565c1b Compare January 4, 2021 17:58
@CheapHasz
Copy link
Contributor Author

I fixed the cs, which seems to have fixed the phpstan too.

As for the DateImmutableType, it seems to have been added to doctrine in 2019 doctrine/mongodb-odm#2118, which I wouldn't exactly call recent

@alanpoulain
Copy link
Member

But released in the end of May 2020 🙂 https://github.com/doctrine/mongodb-odm/releases/tag/2.1.0

@alanpoulain
Copy link
Member

Anyway, we should have a conflict with MongoDB ODM < 2.1.

@CheapHasz CheapHasz force-pushed the mongodb_date_immutable_filter branch from 0fed86f to b760fd9 Compare January 5, 2021 07:18
@CheapHasz
Copy link
Contributor Author

Is that what makes the PHP 7.4 lowest tests fail ? Does it mean that the DateTimeImmutable support will be postponed until the MongoDB ODM requirement is bumped ?

@CheapHasz CheapHasz force-pushed the mongodb_date_immutable_filter branch from b760fd9 to 60304de Compare January 5, 2021 08:26
@alanpoulain
Copy link
Member

That's right. If the MongoDB part was not experimental, we would have to introduce a non-breaking change, but since it's not the case, I think we can just bump the conflict.

@CheapHasz
Copy link
Contributor Author

I'm not sure to understand your reply, but let me know if I need to do anything more to help that fix get merged

@alanpoulain
Copy link
Member

I've added some commits to fix it. I think it's OK now. For the merge, we need to wait for the stable release of 2.6.

@CheapHasz
Copy link
Contributor Author

Ok thanks

Base automatically changed from master to main January 23, 2021 21:59
@kralos
Copy link
Contributor

kralos commented Feb 11, 2021

@alanpoulain what's holding this one up? last from you was awaiting 2.6 stable however we're already up to 2.6.2

@alanpoulain alanpoulain merged commit d4abacb into api-platform:main Feb 11, 2021
@alanpoulain
Copy link
Member

Thank you @CheapHasz.

@kralos
Copy link
Contributor

kralos commented Feb 16, 2021

@alanpoulain I'm assuming main = 2.7? are MINOR releases on api-platform time based? Should we add something more helpful explaining the schedule on https://api-platform.com/docs/extra/releases/#the-release-process

We don't really know what "when they are ready" means. Does it mean there is no schedule? Based on historical MINOR tags we're guessing it's roughly yearly?

With regards to this particular merge, we are now trying to decide if we should wait for 2.7 or revert our application to use DateTime / date instead of DateTimeInterface / date_immutable...

@alanpoulain
Copy link
Member

Yes, main will be the 2.7 version.

Yes, there is no schedule. It depends of the features we have, the time it needs to release a new version, the timing of the release (for instance with a new version of Symfony or PHP), etc.

I'm not sure what's the best choice in your case. The 2.6 version has just been released, so the 2.7 version will take time to be out. You can also use directly the main branch in the meantime.

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.

3 participants