WI, USA
Account created on 12 December 2012, over 12 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States mglaman WI, USA

I've got a MutationObserver working, making MR shortly

🇺🇸United States mglaman WI, USA

Oh, and this effects anything using dynamic code components – menus for example.

🇺🇸United States mglaman WI, USA

I think I hit this with a "simpler" scenario. I have a button link component within the slot of a another component. The page is then visited within the iframe. The problem does not occur if the button link is on its own and not in a slot.

🇺🇸United States mglaman WI, USA

This isn't just about autosave_form. There is a 500 error when rendering the page and the page causes a subrequest.

🇺🇸United States mglaman WI, USA

I ruined the MR. It conflicted on tests. Uploading patch we were using to recreate

🇺🇸United States mglaman WI, USA

The same problem exists for delete links `.action-link`

🇺🇸United States mglaman WI, USA

😮 it finally landed! first work started 4 February 2025 and now we're here 🥳

Thanks to everyone who landed this, from frontend work to backend refinement.

🇺🇸United States mglaman WI, USA

Assigning to @hooroomoo as we're mostly on the front end work now and they are working on it

🇺🇸United States mglaman WI, USA

I don't think there is any harm in removing `$container->getParameter('kernel.environment') !== 'install'` since we're calling `setRebuildNeeded` instead of `rebuild`.

I recently experienced broken routing that may/may not be related.

🇺🇸United States mglaman WI, USA

@siddharthjain do you have the navigation module installed?

🇺🇸United States mglaman WI, USA

Moving back to review. Backend implementation is all set.

🇺🇸United States mglaman WI, USA

Had a discussion w/ tedbow. Keeping NW for now. Idea is to remove staged config update from config POST/GET and make a new auto-save config POST for only staged config update

🇺🇸United States mglaman WI, USA

I think this is good for review again!

🇺🇸United States mglaman WI, USA

The current MR keeps `target` as a property so that each staged configuration update is targeted to one configuration object, and all actions apply to that configuration object.

It would be a non-breaking data model change to allow targeting multiple configuration objects with the actions.

* If `target` is set: do not allow `target` in any `action`
* If `target` is not set: all `action` must specify their `target`

Staged configuration updates live in auto-save only, and there is a compatibility layer possible here if we were to not allow either option and move to all actions specifying their target.

The biggest implication is access checks now must be performed against each action if we went this route.

🇺🇸United States mglaman WI, USA

What if there was a config action that ensured the component config existed and had to be the first action? That was my workaround for code components

🇺🇸United States mglaman WI, USA

Marking for review of the backend specific MR. There are a few open threads that need discussion

🇺🇸United States mglaman WI, USA

Related to above, do we need to couple the target and ID? Couldn't the ID be anything? For example, for the use case of XB UI setting the homepage, could it be something like xb_set_homepage without being coupled to system.site?

You're right, it can be arbitrary by the frontend. My concern was having multiple staged config updates that possibly overlap (like 3 stacked site name changes) but that would be a client error if they didn't use the same ID. I'll double the ID from the config target so that we can do arbitrary IDs.

🇺🇸United States mglaman WI, USA

I'm about to push changes which allow StagedConfigUpdate to support multiple actions. After I did that, I just thought of a problem: if someone changes the site homepage and also something else in system.site (site name?) how can we reflect that easily in one auto-save. That was actually an issue with the original approach that I wasn't sure how to resolve.

🇺🇸United States mglaman WI, USA

I walked through the xdebug of this. The component was in the `content` region. I think the bug is in the `slots` code

          // Maybe it's not a component, but a slot inside a component.
          foreach ($componentData['slots'] as $slotData) {
            foreach ($slotData['components'] as $slotComponentData) {
              if ($slotComponentData['uuid'] === $componentInstanceUuid) {
                return TRUE;
              }
            }
          }

It was nested 2 or 3 levels deep.

🇺🇸United States mglaman WI, USA

This breaks the site when you try to revert a node:

See \Drupal\node\Form\NodeRevisionRevertForm

class NodeRevisionRevertForm extends ConfirmFormBase {

It doesn't implement the interface like the hook expects:

  /**
   * Implements hook_form_alter().
   */
  #[Hook('form_alter')]
  public function formAlter(array &$form, FormStateInterface $form_state, string $form_id): void {
    if (str_ends_with($form_id, '_revision_revert') || str_ends_with($form_id, '_revision_revert_confirm')) {
      assert($form_state->getFormObject() instanceof EntityFormInterface);
🇺🇸United States mglaman WI, USA

This may have fixed a bug but also broke a nice feature with Recipes using Code Components.

   // @todo Fix upstream core bug in Recipes: it inconsistently claims to be
    // syncing when installing modules, but not when installing configuration.
    // Even though it is listed under `import`, and that should hence match the
    // behavior of the /admin/config/development/configuration/single/import UI.
    if (in_array('installRecipeConfig', array_column(debug_backtrace(), 'function'), TRUE)) {
      // Assert the bug is still present. This will start failing as soon as the
      // upstream bug is fixed.
      assert(!$this->configInstaller->isSyncing());
      return;
    }

It was nice not having to provide the component config for code components within a recipe.

🇺🇸United States mglaman WI, USA

Yeah that's fair. So instead of action and input as top properties there'd be a new property that is a sequence of those. I will make that change tomorrow.

I'll probably finish implementing the tests first since that'll be faster and refactor the approach for multiple actions.

Right now the way the ID would be constructed assumed a lot of manual concatenation of target and action via the UI possibly

🇺🇸United States mglaman WI, USA

mglaman changed the visibility of the branch 3503412-set-page-as-homepage-simplify to hidden.

🇺🇸United States mglaman WI, USA

Okay, stepping up for Round 3 on this thing. I like #36 vs a config entity wrapping/representing the simple config. Also possibly easier to display change in pending changes.

🇺🇸United States mglaman WI, USA

Flagging for tests. There must be existing tests that have missing coverage.

🇺🇸United States mglaman WI, USA

The problematic entities were config entities which don't support revisions. Not the actual entity being edited itself. I don't know if this effects regular conponen config for SDCs

🇺🇸United States mglaman WI, USA

XB crashed before due to the firehose widget. Our team uses this hook:

/**
 * Implements hook_form_FORM_ID_alter().
 */
function OURMODULE_form_xb_page_form_alter(array &$form, FormStateInterface $form_state): void {
  $form['metatags']['widget'][0]['#type'] = 'container';
  $form['metatags']['widget'][0]['basic']['#type'] = 'container';
  $form['metatags']['widget'][0]['basic']['description']['#access'] = FALSE;
  $form['metatags']['widget'][0]['basic']['abstract']['#access'] = FALSE;
  $form['metatags']['widget'][0]['basic']['keywords']['#access'] = FALSE;
  $form['metatags']['widget'][0]['preamble']['#access'] = FALSE;
  $form['metatags']['widget'][0]['tokens']['#access'] = FALSE;
  $form['metatags']['widget'][0]['intro_text']['#access'] = FALSE;
  $form['metatags']['widget'][0]['image_help']['#access'] = FALSE;
}
🇺🇸United States mglaman WI, USA

getResourceByURL works for me. Just some way to make requests using link values and getting a serialized response.

🇺🇸United States mglaman WI, USA

We have code which changed the JavascriptComponent to `ContentCreatorVisibleXbConfigEntityAccessControlHandler` to fix a bug in 0.2 and I forgot when we upgraded to 0.4. So it works as expected once I do our change.

🇺🇸United States mglaman WI, USA

Per #10, let's postpone this on 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active . Chatted w/ @tedbow and the conclusion was to postpone since both touch buildPreviewRenderable, then reopen to test.

🇺🇸United States mglaman WI, USA

Marking NR to get feedback on MR questions.

🇺🇸United States mglaman WI, USA

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

🇺🇸United States mglaman WI, USA

Reviewing this, I see

Just to make sure I understand, the limitation is that these test GET for the collection endpoint, then POST, PATCH on the individual entities and also its auto-saves.

So when testing the first list we can do a GET to verify access? Since the OpenAPI schema has been addressed in another issue.

🇺🇸United States mglaman WI, USA

Picking this up.

🇺🇸United States mglaman WI, USA

Marking needs review so we can discuss if cacheability is needed for a non-cacheable HTTP method

🇺🇸United States mglaman WI, USA
$ composer config minimum-stability dev
$ composer require drupal/core-dev "drupal/experience_builder @dev" drush/drush --with-all-dependencies
In PathRepository.php line 163:
                                                                               
  The `url` supplied for the path (../experience_builder) repository does not  
   exist                                                                       
                                      

I see this as a a not-us-failure due to an experimental job. I'll move back to RTBC and someone else can decide different. Re-running the job.

🇺🇸United States mglaman WI, USA

Looks good to me! Tests now reflect the fact the permission is not needed.

Seems like the collection_permission should be a follow up discussion since _user_is_logged_in isn't a blocker. And I know XB has gone for less verbose permissions and adding a collection permission for each config would break that paradigm.

🇺🇸United States mglaman WI, USA

Since 🐛 ContentCreatorVisibleXbConfigEntityAccessControlHandler's `view` access must refuse access to disabled config entities Active landed, can we close this as outdated because the access handler has changed? Per #18 & #20 it seems like it?

🇺🇸United States mglaman WI, USA

This is postponed because we added lookup keys, right? Can we un-postpone this ticket is adding lookup keys for performance was spun out and blocked on that other ticket? That way we land the new access handlers albeit with a performance hit until lookup keys added?

Edit: we can still keep condition check but just not check lookup keys, so same as it was before

🇺🇸United States mglaman WI, USA

RTBC, rerolled @penyaskito work. Called out a few items to discuss which look OK.

🇺🇸United States mglaman WI, USA

+1 use ContentCreatorVisibleXbConfigEntityAccessControlHandler, that organically happened in 🐛 ContentCreatorVisibleXbConfigEntityAccessControlHandler's `view` access must refuse access to disabled config entities Active

🇺🇸United States mglaman WI, USA

One thought I had was keeping a hash, but making it a hardcoded salt. So v0, v1, v2, etc. Whenever XB has a breaking change, this could be bumped and an upgrade hook written. No idea _why_ a breaking change could happen, but it is atleast an option to load different files?

🇺🇸United States mglaman WI, USA

Actually, @apotek if script-src-elem isn't configured, should we default by populating it with the values of script-src? We encountered this because FF was failing but no other browsers due to missing script-src-elem even though we have script-src

🇺🇸United States mglaman WI, USA

I'll be picking this up.

Production build 0.71.5 2024