🇺🇸United States @mglaman

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.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

Oversight whoopsies, approved the MR

https://www.drupal.org/project/cors_ui/releases/8.x-1.3 → just published

griffynh → credited mglaman → .

mglaman → created an issue.

I'll tag a release today with this fix

Thanks! I didn't see this issue until now

I appreciate it! If someone wants to port this to a Cypress test, since I may not be able to until next week, here's my example test:

    await login(page, ...getAdminCredentials())
    await page.goto('/homepage')
    await clickTopBarButton(page, 'Edit')
    await page.getByRole('button', {name: 'Dynamic components'}).click()
    await page.getByRole('button', {name: 'System dynamic components'}).click()
    await expect(page.locator('[data-xb-component-id="block.system_branding_block"]')).toBeVisible()
    await page.locator('[data-xb-component-id="block.system_branding_block"]').click()

    await page.getByTestId('xb-primary-panel--layers').click()
    await page.locator('.primaryPanelContent').getByText('Site branding block').click({ button: 'right' })
    await page.getByText('Move to global region').click()
    await page.locator('[role="menuitem"]').getByText('Header').click()
    await expect(page.locator('.primaryPanelContent').getByText('Site branding block')).not.toBeVisible()
    await page.locator('.primaryPanelContent').getByText('Header').dblclick();
    await expect(page.locator('.primaryPanelContent').getByText('Site branding block')).toBeVisible()

This in Playwright, not Cypress, so it isn't using any @ aliases available

mglaman → created an issue.

MR approved

MR fixed!

Need to fix fails

This partially solves the problem.

\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::findTargetForProps has the field hardcoded in an entity query

    // TRICKY: this is tightly coupled to `media_library_storage_prop_shape_alter()`!
    $query = $this->entityTypeManager->getStorage('media')->getQuery()->condition('field_media_image.target_id', $file_id)->accessCheck();

Otherwise the tests which have it hardcoded should be fine.

mglaman → created an issue.

Sounds good. Attaching the patch for others who need it.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

Looks like the same ask in ✨ Changes to session breaks access to masquerading user. Needs review but titled better

balintbrews → credited mglaman → .

MR up but test needs to be finished

Delete all doesn't exist and was just an idea pitched on #3504970-3: Publish all changes within a database transaction to allow rollbacks →

mglaman → created an issue.

mglaman → created an issue.

Thanks, everyone.

Approved by all codeowners

I read this and was confused, thinking it was the Page entity title, adding block to issue title

Needs tests, but we have a basic code change that could use review and feedback

Pushed https://git.drupalcode.org/project/experience_builder/-/merge_requests/6..., I can update the @todo when it is added. This feels like a perfect case for content moderation and a specific workflow. If the state is archived, we will not make published. Or if we do not want content moderation, it's another use for a trinary state field: 0=TO_BE_PUBLISHED, 1=PUBLISHED, 2=ARCHIVE. On "Publish all changes" all items in 0=TO_BE_PUBLISHED are published.