Overview

In ClientServerConversionTrait we're currently doing this

$props_prepared_for_saving = [];
    foreach ($props as $component_instance_uuid => $component_instance_props) {
      foreach ($component_instance_props as $prop_name => $prop_source) {
        $props_prepared_for_saving[$component_instance_uuid][$prop_name] = json_decode((string) $prop_source, TRUE);
      }
    }

This relies on the `__toString` method of propsources which transforms the value into JSON and then we immediately `json_decode` it.

Proposed resolution

Add a normalize method to each of the prop source classes and use that instead of casting to and from JSON to get an array representation

User interface changes

Issue fork experience_builder-3495129

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

larowlan created an issue. See original summary.

akhil babu’s picture

akhil babu
he/him
commented
Assigned: Unassigned » akhil babu

akhil babu opened merge request !500

akhil babu’s picture

akhil babu
he/him
commented
Assigned: akhil babu » Unassigned
Status: Active » Needs review

As suggested by @larowan, I have added a new method toArray() to the prop source classes

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » wim leers
Status: Needs review » Reviewed & tested by the community

This looks good to me, thanks @akhil babu

longwave made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Assigned: wim leers » larowlan
Status: Reviewed & tested by the community » Needs review

Made some improvements:

  • toArray() is abstract on the base class (we don't have an interface)
  • __toString() is moved to the base class to avoid repetition
  • Also added JSON_THROW_ON_ERROR to appease PHPStan
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Nice improvement 🙌 - merge on green, re-ran the failing e2e 🤞

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: larowlan » wim leers

Looks good, I can't merge it until @wim leers or @f.mazeikis approves the merge request

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +DX (Developer Experience), +API clean-up

Missing https://phpstan.org/writing-php-code/phpdoc-types#array-shapes.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Add a normalize method to prop sources » Add `toArray()` method to prop sources

Title did not match actual MR.

Also: +1, this is long overdue!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3497866: CI: also test against next minor: 11.2.x aka Drupal 11.x HEAD

Can't run phpstan locally while testing what I'm doing, due to:

$ php vendor/bin/phpstan analyze modules/contrib/experience_builder --memory-limit=256M --configuration=modules/contrib/experience_builder/phpstan.neon  -vvv
Invalid entries in excludePaths:
Path /Users/wim.leers/core/modules/contrib/experience_builder/modules/same_page_preview is neither a directory, nor a file path, nor a fnmatch pattern.

Path /Users/wim.leers/core/modules/contrib/experience_builder/tests/src/Cypress/node_modules is neither a directory, nor a file path, nor a fnmatch pattern.

If the excluded path can sometimes exist, append (?)
to its config entry to mark it as optional.

Fix: remove the first one, because it's obsolete. Make the second one optional.

This must be the new PHPStan version that Drupal 11.x HEAD is installing — CI is pinning to 11.1.x since that is the current minor, and hence it's not spotting this problem. Doing that here is a distraction, so opened #3497866: CI: also test against next minor: 11.2.x aka Drupal 11.x HEAD to do that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs work » Reviewed & tested by the community

Fortunately, the PHPStan shape work was already done — I'd forgotten about that!

I did also find one more place where we can use the new method 🥳

  • wim leers committed dbaa44f4 on 0.x authored by akhil babu 
    Issue #3495129 by wim leers, akhil babu, longwave, larowlan, tedbow: Add...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

@wim w.r.t phpstan locally, I've got a `phpstan.larowlan.neon` and inside it I have 

includes:
	- phpstan.neon
parameters:
    scanDirectories:
      - ../metatag
    excludePaths:
      - tests/src/Cypress/node_modules/flatted/php/flatted.php

I've added this to .git/info/exclude so it doesn't get checked in.

In my pre-commit hooks I've got this (the ../../../bin/phpstan is the path in my setup to the phpstan binary)

../../../bin/phpstan analyse -c ./phpstan.larowlan.neon .

This lets me tweak the phpstan setup without impacting the checked in version.

I've got similar things for phpcs and cspell too, also in my pre-commit

Happy to share if they're useful

Status: Fixed » Closed (fixed)

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