Problem/Motivation

The machine name element passes the entire data from #machine_name to the frontend via drupalSettings. This exposes unnecessary data e.g. the php object or class which is used for the exists-check and it leads to very large objects in the cache (stored by AssetResolver).

Proposed resolution

Whitelist the required properties for machine-name.js

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo created an issue. See original summary.

webflo’s picture

FileSize
869 bytes
dawehner’s picture

Oh wow.
Given that

      $function = $element['#machine_name']['exists'];
      if (call_user_func($function, $element['#value'], $element, $form_state)) {

is used I'm wondering whether we should validate whether something is an actual instance? It would not be able to call non static code at the moment, right?

webflo’s picture

Yeah, actually my patch is wrong. We don't need the class name at all inside of drupalSettings. We should whitelist the properties we need in machine-name.js.

webflo’s picture

Status: Active » Needs review
FileSize
937 bytes

Status: Needs review » Needs work

The last submitted patch, 5: 2614066-5.patch, failed testing.

webflo’s picture

Issue summary: View changes
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2614066-5.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
957 bytes
webflo’s picture

Issue tags: +rc eligible
jibran’s picture

Issue tags: +JavaScript

I think we can backport this to D7 as well. Can we add some kind of tests for this? Patch itself doesn't touch JS but I think it'd great if JS maintainer can have a look at it so tagging.

webflo’s picture

webflo’s picture

The last submitted patch, 12: 2614066-12-testonly.patch, failed testing.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Render/Element/MachineNameTest.php
@@ -42,4 +46,76 @@ public function providerTestValueCallback() {
+    /** @var \Drupal\Core\Form\FormStateInterface $form_state */
+    // $form_state = $this->prophesize(FormStateInterface::class)->reveal();

Let's drop those 2 lines then? I think its right to use the value object instead of a mock here.

webflo’s picture

dawehner’s picture

Issue tags: -rc eligible +rc target triage

Thank you.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

.

catch’s picture

Issue tags: -rc target triage +Performance

Fix looks right.

While this changes what's in drupalSettings, it should be impossible for contrib or custom JavaScript to rely on that, so I think it's fine for 8.0.x.

Tagging performance due to the cache entry bloat.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2614066-16.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: 2614066-16.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
webflo’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x, thanks!

  • catch committed 30471f5 on 8.1.x
    Issue #2614066 by webflo: Machine name element leaks to much data to...

  • catch committed 22c7751 on
    Issue #2614066 by webflo: Machine name element leaks to much data to...

Status: Fixed » Closed (fixed)

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