🇺🇸United States @mglaman

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.

mglaman → created an issue.

I opened https://git.drupalcode.org/project/drupal/-/merge_requests/11633 to prove where a fix _could_ go but I have no idea how to solve it. Moving to the asset library system, as I think it belongs there. The problem is how theme libraries are treated special and break the idea of being a dependency

So my dream in doGetDependencies doesn't work. But any adjustments there do not matter because the code path ends up at \Drupal\Core\Asset\AssetResolver::getCssAssets.

    foreach ($libraries_to_load as $library) {
      [$extension, $name] = explode('/', $library, 2);
      $definition = $this->libraryDiscovery->getLibraryByName($extension, $name);
      foreach ($definition['css'] as $options) {

The library definition is fetched here with the group CSS_AGGREGATE_THEME.

And to make sure I'm understanding this correctly, because I've flipflopped in my head a few times: if a theme's library is a dependency for a component, that theme's library should not be in CSS_AGGREGATE_THEME to ensure proper ordering.

It feels like something we'd have to fix in \Drupal\Core\Asset\LibraryDependencyResolver::doGetDependencies. That if a library from a theme is resolved as a dependency. The CSS files should be iterated over and the group changed to CSS_AGGREGATE_DEFAULT.

mglaman → changed the visibility of the branch 3374901-all-for-you-mherchel to active.

mglaman → changed the visibility of the branch 3374901-all-for-you-mherchel to hidden.

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

+1 to this approach! I had forgotten this issue and proof of concept existed when I had a chat with @phenaproxima where we chatted over this idea. The outcome happened to be nearly the same as this issue, except I didn't consider the possibility of having the config entity implement EntityViewDisplayInterface so we could swap it for the view display object.

I pushed a test and some wonky code to further explore. I like that it mimics how page regions work. If something exists we execute a new code path versus embedding into the existing code path.

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

+++++++1 for default value being a placeholder. I've placed a paragraph component so many times and starting typing only to start adding letters after the example content.

However, that's really hard for non-text inputs. In that case I think it's fine to render the example as the default but leave the form empty.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

Let me clear that haphazard comment up, sorry!

I opened this for the OpenAI provider because it duplicates the dependency on `key` from the `ai` module and that module's provider doesn't even leverage code from Key provided by the base provider class. I'm 99% certain it's the same case for the AWS Bedrock Provider as pointed out in #3.

There is some triage which could be done in the modules and it doesn't have to just happen here as a big refactor. That's why I didn't open an issue here to start with. But this could be a plan/meta and then the provider modules adjust accordingly.

It's still valid for every provider that defined a dependency on Key but never uses it.

mglaman → created an issue.

Thanks!

  $importer = \Drupal::service(Importer::class);
  assert($importer instanceof Importer);
  $importer->importContent(
    new Finder(__DIR__ . '/content'),
    account: new AnonymousUserSession()
  );

Like a charm :)

@charlliequadros as per the summary, you need to programmatically import content when 1) there are no roles with `is_admin` flag and 2) super user is disabled. This can be done by importing a recipe with default content or custom content

Needs tests. But I at least wanted to get the idea in an MR before the weekend

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

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