Problem/Motivation

#2310255: [meta] Remove ArrayAccess from FormState is the issue to remove ArrayAccess. But if we want to safely do that in 8.1.x, we have to document it now.

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#9 form_state-2313615-9.patch1.25 KBtim.plunkett
#1 form_state-2313615-1.patch1.25 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new form_state-2313615-1.patch1.25 KB
xjm’s picture

xjm
she/her
Primary language English
commented

I'd also add a note to the docblock of the class itself documenting that the ArrayAccess support is interim, since people using ArrayAccess don't actually call the methods.

I'd support this change; wherever possible, we should use FormState's methods rather than accessing it as an array, as described in https://www.drupal.org/node/2310411. We should probably get confirmation that this is okay to remove in 8.1 when explicitly deprecated now. (I don't think it would be an acceptable BC break otherwise.)

catch’s picture

catch
he/him
Primary language English
commented

I'm not sure it's an acceptable bc break in 8.1.x either, but let's mark it deprecated asap either way.

dries’s picture

dries commented

I agree with catch that if we don't remove this before 8.0, then it would be too disruptive to remove in a minor. I also don't want to hold up 8.0 on removing this, nor prevent it from getting removed if it makes it. So, how about we change "will be" to "might be":

@deprecated in Drupal 8.0.x, might be removed before Drupal 8.0.

xjm’s picture

xjm
she/her
Primary language English
commented

Based on the feedback from @Dries and @catch, perhaps #2310255: [meta] Remove ArrayAccess from FormState should be rc deadline instead of filed against 8.1.x currently?

xjm’s picture

xjm
she/her
Primary language English
commented

Oh, and the related question was: What would be the negative impacts of leaving ArrayAccess in as a BC layer until 9.0.0?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Leaving that code will make things harder to refactor internally later, but it does not stop us completely.

#2310255-7: [meta] Remove ArrayAccess from FormState is fine by me, as is the wording @Dries suggested in #4.

But with that in mind, do we really need this issue?

xjm’s picture

xjm
she/her
Primary language English
commented

I definitely think we do, because people developing with beta 1 need to know about this.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new form_state-2313615-9.patch1.25 KB

WFM

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

There's already the @todo in the docblock to remove \ArrayAccess in #2310255: [meta] Remove ArrayAccess from FormState, so I think that covers my feedback in #2. So this is all we need!

chx’s picture

chx commented

Given that

  1. $form_state['values'] is ubiquitous
  2. AFAIK we do not plan/hope to remove any other ArrayAccess
  3. We know how little people actually read doxygen; everyone will just copy $form_state->getValue from wherever they find it in core

I recommend a separate gdo announcement on this .

xjm’s picture

xjm
she/her
Primary language English
commented

Re: #11, that's a good point. Not a g.d.o announcement though; a change record on the issue that actually removes the array access.

chx’s picture

chx commented

Oh well, I tried.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d21c9a4 and pushed to 8.0.x. Thanks!

  • alexpott committed d21c9a4 on 8.0.x 
    Issue #2313615 by tim.plunkett: Clarify the deprecated-ness of...

Status: Fixed » Closed (fixed)

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