Add upgrade note for #1190

[phansys]

Subject

Add missing upgrade note for #1190.

I am targeting this branch, because this change does not affect BC.

Related to #1190.

I'm not adding a changelog about this change since the related PR is not present in any release yet.

[franmomu]

I thought the UPGRADE-x.md file was meant to add things a user has to do in order to upgrade to version x, I see this more in the changelog of the original PR.

[phansys]

I understand your point @franmomu.
My concern is about the change in the behavior for cases where an instance of \DateTimeImmutable is already passed in projects using previous versions.
By instance, the time manipulation done at AbstractDateFilter::filter() was not affecting the passed value if it was an instance of \DateTimeImmutable, but now it does:

SonataDoctrineORMAdminBundle/src/Filter/AbstractDateFilter.php

Lines 67 to 77 in d6fc51c

	// date filter should filter records for the whole days
	if (false === $this->time && ($value['value']['end'] instanceof \DateTime || $value['value']['end'] instanceof \DateTimeImmutable)) {
	// since the received `\DateTime` object uses the model timezone to represent
	// the value submitted by the view (which can use a different timezone) and this
	// value is intended to contain a time in the begining of a date (IE, if the model
	// object is configured to use UTC timezone, the view object "2020-11-07 00:00:00.0-03:00"
	// is transformed to "2020-11-07 03:00:00.0+00:00" in the model object), we increment
	// the time part by adding "23:59:59" in order to cover the whole end date and get proper
	// results from queries like "o.created_at <= :date_end".
	$value['value']['end'] = $value['value']['end']->modify('+23 hours 59 minutes 59 seconds');
	}

[franmomu]

Ah, that makes sense! but should the upgrade note reflect these changes? otherwise looks like just support was added (it is what happened to me).

[VincentLanglet]

We should be more clear about what we should put to the UPGRADE note.
Same issue here sonata-project/SonataAdminBundle#6585

If we add new feature to upgrade note, adding them to the changelog seems useless.

To me, we should just add to the upgrade note things that will need change from the developer like deprecations (or mandatory BC-break)
Bug fix and new feature shouldn't be added.

[phansys]

Personally, I see the upgrade document as the first place where I should go to check if something related to a package stopped to work or if some previous behavior seems to be changed. If I can't find something related there, I check the changelog.

Ah, that makes sense! but should the upgrade note reflect these changes?

I agree, I'll try to update the message today.

[VincentLanglet]

Personally, I see the upgrade document as the first place where I should go to check if something related to a package stopped to work or if some previous behavior seems to be changed.

If something need to be added to the upgrade document, there should always be with a "HOW-TO" then.

[phansys]

If something need to be added to the upgrade document, there should always be with a "HOW-TO" then.

Should we take the following premise as part of the "HOW-TO" you're proposing?

If you are using these filters with instances of \DateTimeImmutable, be aware of this change in order to determine if you must update your implementation.

[VincentLanglet]

If something need to be added to the upgrade document, there should always be with a "HOW-TO" then.

Should we take the following premise as part of the "HOW-TO" you're proposing?

If you are using these filters with instances of \DateTimeImmutable, be aware of this change in order to determine if you must update your implementation.

I would say yes.

But in this case, I don't understand you because \DateTimeImmutable wasn't supported yet.
So why someone would use it with \DateTimeImmutable.

[phansys]

But in this case, I don't understand you because \DateTimeImmutable wasn't supported yet.
So why someone would use it with \DateTimeImmutable.

I'd say that they were already supported (the filter doesn't have any check forbidding the type), but not in a fully or a proper way (they weren't receiving the same treatment as \DateTime instances).
That's why I consider this as a MINOR instead of PATCH, but we could discuss about this point.

[VincentLanglet]

I'd say that they were already supported (the filter doesn't have any check forbidding the type), but not in a fully or a proper way (they weren't receiving the same treatment as \DateTime instances).
That's why I consider this as a MINOR instead of PATCH, but we could discuss about this point.

Ok.

[phansys]

Ah, that makes sense! but should the upgrade note reflect these changes? otherwise looks like just support was added (it is what happened to me).

The note was updated @franmomu 👍

[VincentLanglet]

You may need to rebase

[OskarStark]

@franmomu please give us a final review here, thanks

[franmomu]

Thanks @phansys!