Problem/Motivation

If you call \Drupal\Core\Entity\EntityTypeManagerInterface::getFormObject() twice in the same request, it stores the same result.
As long you just have on form this is not an issue.

Inline entity form embeds a form in another form. In case you then reference the same entity type in a form, this causes getFormObject() to return the same object twice,
so we leak data from the subform to the main form

Proposed resolution

  • add a parameter to getFormObject() to be able to bypass the cache
  • On the longrun never cache the information. It never makes sense to share the information between instances

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 2626548_12.patch2.09 KBslashrsm
#9 interdiff.txt782 bytesdawehner
#9 2626548-9.patch3.96 KBdawehner
#6 2626548-4.patch3.87 KBdawehner
#5 2626548-3.patch3.13 KBdawehner
#2 2626548-2.patch1.48 KBdawehner

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2626548-2.patch1.48 KB

There we go.

webflo’s picture

webflo commented
Issue tags: +Zurich media sprint 2015
webflo’s picture

webflo commented
dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2626548-3.patch3.13 KB

There we go, with a test.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2626548-4.patch3.87 KB

There we go.

The last submitted patch, 5: 2626548-3.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/lib/Drupal/Core/Entity/EntityTypeManagerInterface.php
@@ -68,11 +68,13 @@ public function getListBuilder($entity_type);
+   * @param bool $use_cache
+   *   (optional) Should the form object instance be cached.

as discussed, I think we should document here that this is basically already deprecated and will be removed in 9.x, it's just here for BC.

Before we'd do that, I'd actually talk to @catch about this, maybe he thinks that we don't need this and can just switch?

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2626548-9.patch3.96 KB
new interdiff.txt782 bytes

Updated the docs a bit for that point. Its hard to describe that the deprecated value is the default for now.

catch’s picture

catch
he/him
Primary language English
commented

Yeah what happens if we just remove the static cache?

dawehner’s picture

dawehner
Primary language German
commented

Yeah what happens if we just remove the static cache?

Yeah this is the question ... could this break contrib / custom code out there?

slashrsm’s picture

slashrsm commented
StatusFileSize
new 2626548_12.patch2.09 KB

Let's see what testbot says about this. I will give no info about the contrib/custom, but it is a start.

There might be contrib/custom code that relies on form object always being the same. But I am thinking if that's abusing details of internal implementation? Documentation for getFormObject() says "Creates a new form instance". Not a word about persisting form object after it was initially created.

slashrsm’s picture

slashrsm commented
Version: 8.0.x-dev » 8.1.x-dev

Discussed this with @catch on IRC. He suggested to commit to 8.1.x immediately and leave RTBC for a while for 8.0.x to test it a bit more.

slashrsm’s picture

slashrsm commented

Added Change record draft.

berdir’s picture

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

I agree, I don't think it's an API that we statically cache there. The documentation even explicitly says new.

Lets get this into 8.1.x as suggested by @catch.

  • alexpott committed 35f8de3 on 
    Issue #2626548 by dawehner, slashrsm: The static caching in \Drupal\Core...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2626548_12.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Reviewed & tested by the community

Looks like alexpott just committed this.

slashrsm’s picture

slashrsm commented

Change record published.

  • catch committed e1060ec on authored by alexpott 
    Issue #2626548 by dawehner, slashrsm: The static caching in \Drupal\Core...
catch’s picture

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

Cherry-picked to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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