[PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field

wim leers → credited tedbow → .

Actually, this is AFAICT not blocked.

Crediting @tedbow because his comments on 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed have helped me craft this. 😊

The failing pipeline is due to the rebase done with 0.x otherwise all tests are passing.

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

MR !957 was messed and hence created a new MR with similar changes

deepakkm → closed merge request !957

Based on meeting just now, can we get:

• the failing tests to explicitly skipped on SQLite, and have a comment pointing to the relevant core issue?
• an update to .gitlabci.yml to use MariaDB by default instead of SQLite? (Or MySQLite, whichever is fastest.)

The pipeline passed - https://git.drupalcode.org/issue/experience_builder-3516432/-/pipelines/..., when the work was done. but failed after a rebase done from another person and i may know why because of the major changes introduced in ApiLayoutController.

I'll update the test case for this once.

but the actual update in gitlab file goes to this issue - https://www.drupal.org/project/experience_builder/issues/3518292 ✨ Allow searching for content in the editor navigation Active , where pipeline failing for 1 and passes for another , i'll update the gitlab file for that. Thanks

It failed due to type -> component_id in the tree

This is now good for review. The failing pipeline in cypress test is not part of the changes done in this MR.

but the actual update in gitlab file goes to this issue - https://www.drupal.org/project/experience_builder/issues/3518292 ✨ Allow searching for content in the editor navigation Active , where pipeline failing for 1 and passes for another , i'll update the gitlab file for that. Thanks

and

The failing pipeline in cypress test is not part of the changes done in this MR.

But … this one surely isn't introducing transliteration, so why is this one failing? 🤔 Anyway, because you really want this merge order, I tried to land ✨ Allow searching for content in the editor navigation Active , but couldn't: #3518292-28: Allow searching for content in the navigator → .

So right now there are 3 cypress tests failing and i have no idea why these tests are failing. No idea how i can move forward in fixing those. The major problem i have in setting up my cypress test is the XbSetup is throwing error.

Though looking for a way forward on this.

Assigning to deepakkm, had some MR review feedback

I have no idea now as to how to fix this random cypress failure [component-operation.cy.js] though i reran this test but it still fails and passes locally as shown in the screenshot.

Giving it a quick look over

Looks good to me!

LGTM too

The playwright failure looks legit though?

$ 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.

I spotted several reductions in test coverage without an MR comment that provides guidance why. Looks like @larowlan spotted it too.

Needs follow-up for at least the component instance form route, because the meta doesn't have an issue for it yet: 📌 SdcController cleanup tasks Active . This (IMHO) confirms the need for a route requirement access check, although that too could be deferred to that follow-up if you prefer.

Same thing for the content entity form route, but AFAICT that could trivially be added here — follow-up is fine too.

I think this is ready, assigning to Wim for a final review and possible merge.

The hook_entity_field_access() implementation unfortunately contained multiple bugs … but only due to terrible DX of core's Entity + Field access! 😭 Sorry you had to wrestle through that, @deepakkm.

Per the issue summary, this was extracted from 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed , ~2.5 months ago. A lot has happened since then. Including that issue having landed. The original was that this would land first, which would then allow 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed to take advantage of the infra this added. I disagree respectfully with all comments #41 through #51 on that issue, because they overlooked something crucial: it would've been up to that issue to ensure the "patch" and "post" routes for editing layout (aka the component tree of an entity) respected field access. 📌 Add test coverage for access exception in \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess Active exists, but that's about generic intra-controller field access checking across all edited fields. When possible, the controller should never be executed; access should be denied at the route access level.

So, retitling to reflect that.

And fixing that was trivial, but the fallout of test breakages was not … turns out this ended up crashing the "breadcrumb block" (because it checks the current URL, strips it path-by-path and then eventually starts hitting the new access control added here 🤪) — all because 🐛 Breadcrumb is not rendering with the right context Active is not yet fixed. So, worked around that by forcing the preview to always use "front page" as the breadcrumb.

Marking this to signal the incompleteness. Because:

1. The follow-up needed (as described in #32) still needs creating.

2. This stuff was added way back when I created Pages and didn't realize XB was broken with null entity support. I think we should keep the fix for this test and in a follow up add support for null entity (new entity creation.) There may even be one.

   — @mglaman at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

👉 multiple follow-ups still needed 🙏

Marking this to signal the incompleteness. Because:

1. The follow-up needed (as described in #32) still needs creating.

2. This stuff was added way back when I created Pages and didn't realize XB was broken with null entity support. I think we should keep the fix for this test and in a follow up add support for null entity (new entity creation.) There may even be one.

   — @mglaman at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

👉 multiple follow-ups still needed 🙏

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

Shoot — this caused a regression 😭 I've requeued e2e tests dozens of times today, which is how I failed to spot that these failures were legitimate 😞

The 0.x CI job for this MR landing is consistently failing on:

1. contain-css.cy.js
2. global-regions.cy.js

… which I only noticed after re-queuing those CI jobs multiple times too.

The latter explicitly uses the breadcrumb block (which this MR made better (out of sheer necessity to be able to add the necessary access protection to routes, so reverting is not really an option), the former probably is implicitly relying on it? 🤔 I don't see how yet though.

I'll take a look

wim leers → changed the visibility of the branch 3516432-unbreak-head to hidden.

wim leers → closed merge request !1134

Thanks, @isholgueras, for saving the day! ❤️