🇺🇸United States @mglaman

mglaman → created an issue.

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.

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

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.

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

mglaman → created an issue.

Opened ✨ Allow working with a new (unsaved) entity Active as follow up

mglaman → created an issue.

This work should be done in ✨ Add Experience Builder extension Active because it wasn't merged yet

mglaman → created an issue.

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

mglaman → created an issue.

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!

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

It failed due to type -> component_id in the tree

nicxvan → credited mglaman → .

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.

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

Thanks @alexpott

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

Thanks @alexpott

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

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.

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

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.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

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

mglaman → created an issue.

mglaman → changed the visibility of the branch 1.x to active.

mglaman → changed the visibility of the branch 1.x to hidden.

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

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.