Problem/Motivation

DateTimeFormatter stores the storage in a property which makes injecting it early problematic.

Steps to reproduce

Inject dateformatter in something that runs early.

Proposed resolution

Remove storage from property.
Modernize DI

Remaining tasks

Review
Confirm we can just remove the property or if we need deprecation.

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3559366

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan opened merge request !13920

nicxvan’s picture

nicxvan commented

Do we need to deprecate dateFormatStorage? How do we do that since we populate it in the constructor without it triggering.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
Status: Active » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

We could do a __get() for BC, I'm no sure it's worth it.

FWIW, the added types are also a possible BC issue, I'm pretty sure https://git.drupalcode.org/project/hijri/-/blob/3.0.x/src/HijriFormatter... will break because it for some reason redefines the properties. Feels a bit pointless to worry about the storage property but knowingly break that module?

So either we are extra careful, add BC and don't change any other properties here, or we clean it up and live with a possible break?

nicxvan’s picture

nicxvan commented

I don't think anything will break except the missing storage property i didn't mark anything readonly and they are defining them to the same type.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The example I shared does not use types, so it will fail: https://3v4l.org/Zf54T#vnull

If the parent has a type, you must have one as well. That's the problem with these sort of cleanups.

mstrelan’s picture

mstrelan commented

That module is only compatible with D8/9, are there other examples?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

There are a few that extend DateFormatter, that's the only one that I found that would be affected by this change. But it's just meant as an example that it can be a BC break in some cases. What it does is weird and completely unnecessary, but it can't be ruled out that there's custom code out there that would break too.

Again, my argument is just that we decide we either do full BC, and then we don't include the other constructor changes, or we don't (which is essentially this MR as it is now), not a mix between those options. As a reply to #3.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks fine to me, just the BC question. I'm not sure, like we recently saw with that webform __get() issue on the entity storage handler, it's even possible that attempting to do BC can do more harm than good, even though it's unlikely to be a problem here.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Just hit date.formatter again in a circular reference in Drupal commerce and it seems related. So clear +1 on getting this fixed in core. Thank you @nicxvan and @berdir!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Let's go ahead here. bc for constructors is 'best effort' and sometimes 'best' is 'none'.

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !13920

  • catch committed 5341ebff on 11.x 
    fix: #3559366 Datetime storage should not be stored in a property

By:...

  • catch committed 9d8bdcbf on main 
    fix: #3559366 Datetime storage should not be stored in a property

By:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.