Update Entities to use Doctrine 2.6 types

[gabel]

Tests will fail due to DateTime conversion of handleRequest() (src/Controller/Admin/BlogController.php:84)

[javiereguiluz]

What can we do here to move forward?

• The application is broken after this change. Can we easily fix it?
• What's the practical advantage of using immutable date types in this application?

Thanks!

[jkufner]

What's the practical advantage of using immutable date types in this application?

When you pass the same datetime object to two components and one of them adds a day without cloning, the immutable type won't affect the other component, but the mutable will change in both places. It is quite hard to find such bugs.

[javiereguiluz]

@jkufner yes, that's the theory part. But now, focusing exclusively on practice, which bug does this change fix or what real improvement does it bring to this specific application? Thanks!

[jkufner]

@javiereguiluz When there is no bug, it does not matter which of the types you use, but you can never be sure there is no bug. Immutable types make code more robust and bugs easier to locate. This change will pay off when some one will modify or build on the application and makes a mistake in the process.

[javiereguiluz]

@jkufner yes, in general that's true. But in this specific application, which is the benefit of this change? I'm not saying there's none, I just don't see it and that's why I'm asking.

[jkufner]

@javiereguiluz It is a good practice and a precaution for the future. Something that an example app should highlight. The code itself does the same.

[gabel]

I assume we cannot move forward here until the underlying libs have fixed the immutable usage. If @javiereguiluz sees no benefit for now this PR and #620 may be closed.

[javiereguiluz]

Let's ask other regular contributors of this repository to see if they agree with this change or prefer to close it (and add it back later when things improve): @yceruto @voronkovich @bocharsky-bw. Thanks!

[yceruto]

Expected argument of type "DateTimeImmutable", "DateTime" given

Could not convert PHP value of type 'DateTime' to type 'datetime_immutable'. Expected one of the following types: null, DateTimeImmutable

Sadly the Form Component is not ready completely to return a DateTimeImmutable instance upon reverse transformation:

https://github.com/symfony/symfony/blob/0082981e4135cf6c04545156db8fcda2c9459f9a/src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToRfc3339Transformer.php#L72

So we might have to do something to fix it, probably into DateTimePickerType and then I'm not sure if these changes are worth it.

[yceruto]

Should be \DateTimeImmutable to avoid \DateTime instances, same for Comment::setPublishedAt().

[yceruto]

Otherwise, we can convert here to immutable date time if needed:

if ($publishedAt instanceof \DateTime) {
    $publishedAt = \DateTimeImmutable::createFromMutable($publishedAt);
}

$this->publishedAt = $publishedAt;

Same for Comment::setPublishedAt().

[voronkovich]

@yceruto, What about to create a data transformer for those properties?

[yceruto]

Yes, it could be so, but still I'm not sure if it's worth it right now.

[voronkovich]

I agree with @jkufner that the usage of the DataTimeImmutable makes an application more robust. So, I think we should wait until the Form component has been fixed.

[yceruto]

Should be \DateTimeImmutable.

[javiereguiluz]

If the Form component doesn't support immutable dates, that's a huge blocker for me. Is there any open pull request to improve the Form component about this? Will it be merged soon? If not, I'm afraid we should close this as "can't fix" until the Form component improves.

[gabel]

As already mentioned in #620 - I guess that somebody has to pick up this unfinished PR symfony/symfony#19889 before it makes sense to continue here.

[voronkovich]

@javiereguiluz, @gabel we could use a datetime_immutable type only for the Comment::publishedAt property.

[javiereguiluz]

Let's close this until Symfony Forms fully support immutable dates (which sadly could take a lot of time!) Meanwhile, I've extracted in #669 the part of this pull request unrelated to immutable dates. Thank you all for the discussion.