I've got a MutationObserver working, making MR shortly
Oh, and this effects anything using dynamic code components – menus for example.
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.
This isn't just about autosave_form. There is a 500 error when rendering the page and the page causes a subrequest.
I ruined the MR. It conflicted on tests. Uploading patch we were using to recreate
The same problem exists for delete links `.action-link`
wim leers → credited mglaman → .
balintbrews → credited mglaman → .
😮 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.
Assigning to @hooroomoo as we're mostly on the front end work now and they are working on it
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.
MR feedback addressed
@siddharthjain do you have the navigation module installed?
Moving back to review. Backend implementation is all set.
balintbrews → credited mglaman → .
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
I think this is good for review again!
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.
🎉
Works for me!
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
Marking for review of the backend specific MR. There are a few open threads that need discussion
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.
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.
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.
tim.plunkett → credited mglaman → .
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);
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.
phenaproxima → credited mglaman → .
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
mglaman → changed the visibility of the branch 3503412-set-page-as-homepage-simplify to hidden.
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.
Flagging for tests. There must be existing tests that have missing coverage.
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
🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active just landed.
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;
}
getResourceByURL works for me. Just some way to make requests using link values and getting a serialized response.
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.
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.
Marking NR to get feedback on MR questions.
Reviewing this, I see
::testJavaScriptComponent: https://git.drupalcode.org/project/experience_builder/-/blob/0.x/tests/s...::testAssetLibrary: https://git.drupalcode.org/project/experience_builder/-/blob/0.x/tests/s...
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.
Picking this up.
Does this duplicate 🐛 XbConfigEntityHttpApiTest Active or just would have been found by it.
LGTM!
mglaman → created an issue.
Marking needs review so we can discuss if cacheability is needed for a non-cacheable HTTP method
Should this be postponed on 📌 Allow XB to be used on all node types Active
Opened ✨ Allow working with a new (unsaved) entity Active as follow up
This work should be done in ✨ Add Experience Builder extension Active because it wasn't merged yet
Looks good to me!
penyaskito → credited mglaman → .
penyaskito → credited mglaman → .
tim.plunkett → credited mglaman → .
Looks good to me! Combines needed fixes from https://git.drupalcode.org/project/jsonapi_query_builder/-/merge_requests/1 and https://git.drupalcode.org/project/jsonapi_query_builder/-/merge_requests/2 plus more fixes.
$ 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.
Looks good to me!
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.
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?
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
CI failing but Wim made good fixes, I'll address
balintbrews → credited mglaman → .
RTBC, rerolled @penyaskito work. Called out a few items to discuss which look OK.
+1 use ContentCreatorVisibleXbConfigEntityAccessControlHandler, that organically happened in 🐛 ContentCreatorVisibleXbConfigEntityAccessControlHandler's `view` access must refuse access to disabled config entities Active
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?
mglaman → changed the visibility of the branch 0.x to hidden.
Giving it a quick look over
balintbrews → credited mglaman → .
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
I'll be picking this up.
Assigning to deepakkm, had some MR review feedback
Making minor
Should the parent be 📌 SdcController cleanup tasks Active so it can be properly tracked?