Decide on future of ArrayAccess support in DataDefinition

Looks there's no way to remove it, so removal of todo probably is enough

Removal of todo seems good.

It looks like even the typed data internals rely on the array access, like for example \Drupal\Core\Config\TypedConfigManager::buildDataDefinition. I guess if we could get rid of the typed data system not internally using array access, we could get a real list of test fails.

If we decide that's not worth it, I wonder if we should remove the "This is for BC support only." comment too.

Agreed with @lauriii.

I don't understand why it'd make sense to keep BC support forever and simply remove the @todo.

Let's trigger a deprecation error and observe what fails?

Let's trigger a deprecation error and observe what fails?

All patches in this issue did exactly that.

Result from #18: https://dispatcher.drupalci.org/job/drupal_patches/203233/#showFailuresLink

Test Result (2,578 failures / +2576)

+2576

It means ~10% so challenging but doable

Spokje → closed merge request !4766

Closed MR now other direction is chosen, updated IS.

the real goal

updated IS

Drafted CR → to extend with examples and created MR to iterate

In MR I started deprecation, looks it also will need API addition

offsetUnset() - has no usage in core but has not replacement even using definition's methods,
because, for example DataDefinition::setConstraints(), require array so not useful to unset key in $this->definition

- offsetGet() is replaceable with toArray(), example fixes in commit

btw there's problem with toArray() - it is not defined by any interface(

probably needs separate issue

The only tricky place is offsetSet() which has only one usage in \Drupal\Core\Config\TypedConfigManager::buildDataDefinition() which could be changed

OTOH createFromDataType() has 5 overrides and could change signature to pass initial values so the raw setter is not needed

Seems like this change was definitely planned to be made given the inline comment. Regardless of that it would be great to get a framework manager review on this. 😇

Couple of comments on the deprecation method itself but I think the overall change is fine. That we only have to update code in the config system and config_translation module suggests this won't affect a lot of contrib modules.

Moving to needs work to get the feedback addressed.

Rebased and squashed commits, updated deprecation wording and 10.4 core

maybe it should be deprecated in 11.1?

Tests are green and looking at changes there are only properties nullable, translatable, mapping

Also mapping property used to validate requiredKey so replaced with a hack to track changes in array processing, probably need Wim Leers to review

Updated to use static::class to get the real data-class name

legacy test added

Reviewing the MR appears all threads have been addressed.

Deprecation appears to have test coverage.

Change record appears clear, also examples are always appreciated.

Believe this is ready.

In two minds whether we should even do this given we have multiple workarounds in this MR that look a bit weird, and we've had to add an explicit setter method.

Moving back to needs review, looks like this needs more discussion.

I think we should do it anyway but amount of workarounds are debatable.

Glad to get more opinions

Think we missed the 11.1 window, mind updating to 11.2, feel free to ping me to re-look at.