🇺🇸United States @mglaman

Not a fan of conflating deprecation fixes for PHP 8.4 with other coding standards cruft, but this would be good to get in

Normally I agree, but given it's just adding ? for nullables, I think it is okay.

mglaman → made their first commit to this issue’s fork.

Why does the URL have ?ajax-form? That's not part of the normal action link. Is there another contrib at play which AJAX-ifies forms?

Reopening, I think there is a bug and has a quick fix by converting to a static property

Actually, inOverride should be static so that it is set across all instances and not just individual instances of the class.

Actually the inOverride flag should be preventing the recursiveness I described. But sounds like it didn't in the original IS

I don't know why this isn't breaking more, but KeyConfigOverrides::getMapping is loading multiple config. Which loading config applies overrides. And that is then re-running KeyConfigOverrides

I want to push this forward but I honestly have no idea how.

Field access is already check in \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess() which throws a AccessException. Add test coverage to prove the ApiLayoutControllerPostTest to prove it throws an 4xx

We dove into this and found out that 4xx will never be thrown, due to Form API ignoring the values. However, the values are still in autosave. So the approach was to just always save the lesser of allowed values, but that breaks other tests, see #37

Either update ApiAutoSaveController::post() or add a new route requirement-level access check (TBD which of the two is best), which must:

Was that done here or still needs to be done? I am unsure this this issue had some work committed but not closed. The scope is blurred.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

I know Drupal core doesn't have a "needs rebuild" flag for the menu links like it does the router. But this module could implement its own version of that. So on kernel terminate it rebuilds the menu (ideally once per request then) not multiple times per request

Marking NR as there has to be discussion on how this all works. See tests in https://git.drupalcode.org/project/experience_builder/-/merge_requests/973

It appears that with how Form API works, the logic in checkPatchFieldAccess will always return false when there is no access because the field values will always be the same. It will never throw an AccessException.

It looks like Form API is taking care of the field level access for us. The form is built with #access => FALSE for fields without access. So any data passed in is ignored.

In my testing I dug in and noticed \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess never went further than:

    if ($original_field->access('view') && $original_field->equals($received_field)) {
      return FALSE;
    }

So the AccessException was never thrown. And that's because the field is never updated by Form API. I'm not sure how we can test AccessException being thrown since something in Form API would break down.

Here's a screenshot from Xdebug showing the `sticky` field set to `access => false` in the form build array.

Picking this up today and will write the tests

Needs tests. Normally I write a kernel test which implements CacheTagsInvalidatorInterface to track invalidations, so we could have a Page using a JS component. Then re-save the JS component and validate the cache tag invalidated.

But I suppose we could just render a page and verify the JS component cache tags are present now.

mglaman → created an issue.

Looks good to me, fixes a bug with private properties and DependencySerializationTrait

mglaman → created an issue. See original summary → .

Impossible to use unless the user has both permissions

mglaman → created an issue.

mglaman → changed the visibility of the branch 1.x to active.

mglaman → changed the visibility of the branch 1.x to hidden.

mglaman → made their first commit to this issue’s fork.

mglaman → made their first commit to this issue’s fork.

wim leers → credited mglaman → .

Assigning to @wim-leers for "The change requests must be completed or resolved."

📌 Pass current user's XB permissions to the XB UI Active is finished, can the updated and agreed upon format be updated here? Are we using raw permissions from Drupal or should it be createPage, etc because that is not what was done in the other ticket.

./scripts/tag-release.sh
zsh: permission denied: ./scripts/tag-release.sh

bash scripts/tag-release.sh 
Tag to create release for: 3.2222
☢️ Are you sure you want to use 3.2222? Then write it again: 3.22222
⛔ Nope.

Tests are passing now, validation and test cover looks solid.

++ to "The full view mode is created if it doesn't exist."

In Drupal Commerce we didn't implement this properly. We ship "default" view mode and displays for products. So this will self-heal/fix contrib or userland code.

nit: "For nodes, that's the full view mode."

It's not just nodes, technically the canonical view mode is always "full" per the default parameter value of \Drupal\Core\Entity\EntityViewBuilderInterface::view and \Drupal\Core\Entity\EntityViewBuilderInterface::viewMultiple

Only when 0 Block config entities existed!

😂 par for the course for me to hit that oddball edge case.

I consider the MR is 90%. It needs review on the approach.

laurii, larowlan were part of the debugging crew

mglaman → created an issue.

balintbrews → credited mglaman → .

which is unique in that there are enabled global regions but they're all empty.

Seems like it'd be a common place for anyone starting fresh with XB and just turned on their global regions. Great catch

Looks good, and thanks! Makes sense to swallow up the exception for now. If other folks start doing more with recipes and find it useful it can be argued to fail loudly for DX around building recipes.

Thanks for making the MR, had to move on and didn't have time to open one.

🐛 Once previewed in XB an entity with no changes will still show up in "Review x changes" Active introduced this method and possibly missed in the refactor

mglaman → created an issue.

mglaman → created an issue.

Approved!

mglaman → changed the visibility of the branch 3503412-allow-cms-author to hidden.

I've recreated the MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/882

mglaman → changed the visibility of the branch 0.x to hidden.

+1, matches how we discussed – field exists, to a template check, render if template exists, otherwise fallback to normal rendering

Summarizing a call I had with @phenaproxima and @lauriii about what opting into XB rendering truly means. There was debate around a third-party setting on the bundle, a separate config object, or using the existence of a content template for the full view mode. In the end we circled back to the heuristic being: does a component tree field exist for the bundle.

1. Checking for a field definition is reliable and is available on the entity. It does not require loading other entity types (does a template exist? load the bundle's third-party settings) or loading a different config object
2. Templates will allow exposed props. Those exposed props have to be stored in a component tree field. That means we always need a component tree field to exist.

When going to a bundle and opting into XB we will:

1. Create the component tree field for the bundle (create storage if needed for the entity type)
2. Ensure the entity type has a full view mode (core entities do, but contrib/custom may not)
3. Copy the default view display for full (like checking display_modes_custom in EntityDisplayFormBase
4. We will not create an empty content template for full

This allows using XB with the bundle and allowing fallback rendering using the normal view display until the content template has been created.

This ended up in my sprint and need to upload. But I think one of the remaining blockers was the missing 🏠 icon if a page/entity/whatever in the navigation has a path which matches system.site.path.front?

wim leers → credited mglaman → .

phpstan-drupal should be fixed: https://github.com/mglaman/phpstan-drupal/releases/tag/2.0.4

no idea if CI pulls latest or if it needs core to

But the API response does not yet support pagination. Thoughts?

Technically it'd still be a subset of the default result without a query string. But I do think getting pagination is going to become important pretty soon

mglaman → created an issue.

Opened https://github.com/mglaman/phpstan-drupal/issues/852 to take a look

Checking a few things:

phpdoc:

   * @param array $images
   *   Images of the project. Each item needs to be an array with two elements:
   *   `file`, which is a \Drupal\Core\Url object pointing to the image, and
   *   `alt`, which is the alt text.

So PHPStan has no idea what the iterator contains

    assert(
      Inspector::assertAllArrays($images) &&
      Inspector::assertAll(fn (array $i): bool => $i['file'] instanceof Url, $images) &&
      Inspector::assertAllHaveKey($images, 'alt')
    ) or throw new \InvalidArgumentException('The project images must be arrays with `file` and `alt` elements.');

The latest changes to Inspector help do type narrowing so that PHPStan understands the array as if the phpdoc had list<array{file: Url, alt: string}>

1. Inspector::assertAllArrays($images) tells PHPStan $images is array<int|string, array>
2. Inspector::assertAll(fn (array $i): bool => $i['file'] instanceof Url, $images) should tell PHPStan it is array<int|string, array{file: Url> but this seems to break down
3. Inspector::assertAllHaveKey($images, 'alt') should flush out the array shape, but now PHPStan doesn't think $images is an array at all because the previous assert failed

I'll see if I can get a test case in phpstan-drupal and quick fix

I don't 100% love a 3rd party setting, but without one hijacking the "Manage Display" local task could be more difficult. My thought was "if full view mode template exists, the bundle is opted in."

phenaproxima → credited mglaman → .

mglaman → created an issue.

🔥🎉

wim leers → credited mglaman → .

@balintbrews thanks for verifying and quick turnaround for a fix 🔥

Attaching a hacky suggestion made by Copilot. I don't think it is the right approach but it is close to what I was thinking. Before considering the editor ready we need to make sure loading is done and not just trust changes to the main data dependencies to mean loading is complete.

I think this is a real bug and not just due to Redux dev tools. There is a condition in useCodeComponentData that would allow writing bogus data, I think.

Essentially the effect which writes to the autosave checks if any of its dependencies (the components data) has changed. One the first run it assumes the editor isn't ready and then flags the editor as ready. After the timeout it now thinks the editor is ready and writes data. But nothing makes sure that the following items have valid data in them:

    [
      blockOverride,
      compiledCss,
      compiledJs,
      componentId,
      name,
      props,
      required,
      slots,
      sourceCodeCss,
      sourceCodeJs,
      status,
    ],

I recently experienced an issue where the JS component lost all of its data except its label:

(object) array(
   'owner' => '2',
   'data' => 
  array (
    'entity_type' => 'js_component',
    'entity_id' => 'navigation_menu',
    'data' => 
    array (
      'status' => false,
      'name' => '',
      'machineName' => 'navigation_menu',
      'source_code_js' => '',
      'source_code_css' => '',
      'block_override' => NULL,
      'props' => 
      array (
      ),
      'slots' => 
      array (
      ),
      'required' => 
      array (
      ),
    ),
    'data_hash' => 'ebb6b23bf8ad844f',
    'langcode' => NULL,
    'label' => 'Navigation menu',
  ),
   'updated' => 1743575219,
)

It even became disabled. Could this be due to a misfired autosave?

mherchel → credited mglaman → .

mglaman → created an issue.

I just realized this may not work due to how setBackendConfig works:

  #[ActionMethod(adminLabel: new TranslatableMarkup('Set backend config'), pluralize: FALSE)]
  public function setBackendConfig(array $backend_config) {
    $this->backend_config = $backend_config;
    // In case the backend plugin is already loaded, make sure the configuration
    // stays in sync.
    if ($this->backendPlugin
        && $this->getBackend()->getConfiguration() !== $backend_config) {
      $this->getBackend()->setConfiguration($backend_config);
    }
    return $this;
  }

So the parsing cannot be done unless the instance has been instantiated.

mglaman → created an issue.

mglaman → created an issue.

+1 I'm okay with removing administer, we just need an upgrade path for adjusting on existing sites. Or are we going to just let end users handle that if they've decided to long-term run XB?

Took me a while, but I opened https://www.drupal.org/project/drupal/issues/3516482 🐛 ConstraintManager::getDefinitionsByType does not validate definitions and can lead to a crash Active

mglaman → created an issue.

We should keep administer pages and follow normal permission patterns. Otherwise users will need to have edit, create, and delete instead of one permission.

I noticed that the administer xb_page permission isn't used for anything at the moment, and I also couldn't think of anything that we should move behind that permission at the moment. We will have to introduce this permission later, once we have page entity specific configurations in the UI.

It is. It is the edit/create/delete permission. Removing this makes the granular permissions more complicated. Someone may want authors to have full permissions whereas some may only want create/edit but no delete.

Note: this means the Page content entity type will NOT use the node module's access content.

I think this is wrong in issue summary as we are using it, but it's provided by the system module.

We made a change to avoid doing 🐛 LibraryDiscoveryParser overwrites theme CSS group ordering Active and exposing group as an API.

Started, then realized this adds new interfaces and makes it a major version change.

So a bunch of performance tests bumped in their stylesheet counts, which I think is a consequence of the different group. It is documented as a reason in 📌 Remove separate CSS_AGGREGATE_THEME aggregate file Needs work . Some of the jumps seem high, I'm not sure if this is expected or can be mitigated without #1924522

\Drupal\Core\Entity\EntityDisplayRepository::getViewDisplay and \Drupal\Core\Entity\EntityDisplayRepository::getFormDisplay also have code the currently exist in these methods, where a fallback new display entity is created.

This would make a concept in Experience Builder much easier to implement.

📌 move static collect*() methods from display entity classes to EntityDisplayRepository Active would make this much easier. Then we could swap the display objects not and need hook_entity_prepare_view and hook_entity_view_alter

Okay. This will need a change record if it goes through along with the issue summary update and a title update.

SDCs libraries are now aggregated at the theme group

Per #36 we can't make SDCs have a specific group without 🐛 LibraryDiscoveryParser overwrites theme CSS group ordering Active

Chatted w/ laurii about #45 and got a thumbs up with having SDCs grouped into the theme aggregate versus default

Chatted w/ laurii about #45 and got a thumbs up with having SDCs grouped into the theme aggregate versus default

The other solution and proposal: all SDCs should have their libraries in the theme aggregation group since they're theme level anyway

There is to this in App.js

https://git.drupalcode.org/project/jsonapi_query_builder/-/blob/1.0.x/js...

I don't know why jsonapi is hardcoded here. Especially when a URL object could be used to pop the path from the absolute URL passed in. It's also duplicated in https://git.drupalcode.org/project/jsonapi_query_builder/-/blob/1.0.x/js...

mglaman → created an issue.