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?
Given we have 📌 Add test coverage for access exception in \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess Active and 🐛 Fields values the user has view but not edit access can be saved to AutoSave Needs work can this issue be closed?
Marking needs review, thanks for fixing the tests @budalokko. https://git.drupalcode.org/project/jsonapi_menu_items/-/merge_requests/16 seems good, but extra test coverage would be great
Great catch!
It failed due to type -> component_id in the tree
If I'm understanding correctly, a content template could have 12 exposed slots. Which means 12 fields will be programmatically attached to that bundle, and all the implications of dedicated field storage. Right?
In an interim I wonder if we can just add `theme` as proposed and in \Drupal\Core\Recipe\RecipeRunner::toBatchOperationsRecipe we had a matching method which runs the command over CLI. The biggest problem would be timeouts since we cannot batch it into smaller operations. Is that acceptable?
I started working on this and realized that it will cause a whole bunch of internal things needing to be exposed as public static methods and also cause us to lose some of the verbosity output when generating a command.
There is also another issue if the spun out code uses TranslatableMarkup because it requires a container and the GenerateTheme command was made specifically to work without an installed Drupal site.
I think this should be a series of issues, but going for a "big bang" fix just to prove its possible.
Thanks @alexpott
mglaman → made their first commit to this issue’s fork.
Thanks @alexpott
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.
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.
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