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

BUGFIX: Properly parse `DateTimeImmutable` types #1442

Merged
merged 2 commits into from Nov 25, 2018

Conversation

Projects
None yet
6 participants
@albe
Copy link
Member

albe commented Nov 16, 2018

Currently, a type string of DateTimeImmutable will be parsed as DateTime. This, for example, leads to any entity properties annotated as @var DateTimeImmutable to be hydrated into a DateTime class instead.

Note that this is a hotfix at most, because the real issue is, that the regex does not check for a type ending character, like whitespace, line end or another non-word character. Therefore, it eagerly parses DateTimeImmutable by matching the DateTime prefix and taking that as the parsed type, which is wrong.

BUGFIX: Properly parse `DateTimeImmutable` types
Note that this is a hotfix at most, because the real issue is, that the regex does not check for a type ending character, like whitespace, line end or other non-word character. Therefore, it eagerly parses `DateTimeImmutable` by matching the `DateTime` prefix and taking that as the parsed type, which is wrong.

I'd rather change the regex to end with `(:?\B?|$)`, but I'm unsure how breaking that would be.
@albe

This comment has been minimized.

Copy link
Member Author

albe commented Nov 16, 2018

Uncovered via #1401

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 16, 2018

Could you describe the bug, I don't get it

@albe

This comment has been minimized.

Copy link
Member Author

albe commented Nov 16, 2018

Added a test for the wrong behaviour. Still this should be handled more generally rather than just for the single DateTimeImmutable case.

@kdambekalns
Copy link
Member

kdambekalns left a comment

Fine with me. Regarding boundaries here, this is supposed to get a "type-only string" as input, so there must be nothng else in it anyways.

@skurfuerst
Copy link
Member

skurfuerst left a comment

makes sense for me so far. I'd say appending the regex with "$" should be fine, though this might have bigger implications so that patch should IMHO be tested on real world projects if needed beforehand.

So first let's get this in I'd say :)

@ComiR

This comment has been minimized.

Copy link
Contributor

ComiR commented Nov 20, 2018

Why even define it with all those extra options? ^\\\\?(?P<type>[a-zA-Z0-9\\\\_]+)(?:<\\\\?(?P<elementType>[a-zA-Z0-9\\\\_]+)>)? should suffice (since [a-zA-Z0-9\\\\_]+ matches the other options as well...).

@albe

This comment has been minimized.

Copy link
Member Author

albe commented Nov 20, 2018

That's my thought too. But let's get this fixed quickly for now like Sebastian suggests and then discuss how to solve this more generally in an own issue/PR.

@jonnitto jonnitto merged commit f92ec40 into 4.3 Nov 25, 2018

4 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@albe albe deleted the albe-typehandling-fix branch Nov 25, 2018

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 29, 2018

That change seems to break doctrine entities with DateTimeImmutable properties:

Failure while evaluating property annotations for class "Neos\Media\Domain\Model\ImportedAsset": The attribute 'type' is required for the column description of property Neos\Media\Domain\Model\ImportedAsset::$importedAt

(Neos master, doctrine ORM 2.6.3)

@albe

This comment has been minimized.

Copy link
Member Author

albe commented Nov 29, 2018

This basically just oncovered the buggy type parser. This in itself led to DateTimeImmutable types no longer being automatically mapped to DateTime types in Doctrine. With #1401 being applied this is fixed. Did upmerges so master is not breaking any longer.

Edit: Wait, this BUGFIX is in 4.3, but #1401 is only in 5.0+... means we basically have a "breaking" 4.3 patch release up our sleeves...

@ComiR

This comment has been minimized.

Copy link
Contributor

ComiR commented Nov 29, 2018

Fortunately it's not released yet. Since doctrine/orm only introduced date_immutable in version 2.6, so we cannot backport #1401 (due to Flow 4.3 only requiring doctrine/orm ~2.5.0).
So either we fix much more than "only" this, or implement this fix only in 5.0+...

@kdambekalns

This comment has been minimized.

Copy link
Member

kdambekalns commented Nov 29, 2018

Since #1401 is in 5.0+ only, this one should be reverted for 4.3 (but kept in 5.0+).

kdambekalns added a commit to kdambekalns/flow-development-collection that referenced this pull request Nov 30, 2018

BUGFIX: Revert merge neos#1442 from neos/albe-typehandling-fix
This reverts commit f92ec40, reversing
changes made to a00a751.
@kdambekalns

This comment has been minimized.

Copy link
Member

kdambekalns commented Nov 30, 2018

I created a "revert PR", please have a look. I think I untangled this mess correctly… :)

albe added a commit that referenced this pull request Nov 30, 2018

Merge pull request #1466 from kdambekalns/bugfix/revert-1442
BUGFIX: Revert merge #1442 from neos/albe-typehandling-fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.