🇺🇸United States @tedbow

Test are failing because of 📌 Since twig/twig 3.12: Twig Filter "spaceless" is deprecated Active which has a fix awaiting approval

@wim leers thanks for the feedback and @larowlan thanks for addressing those problems,.

@wim leers see the update summary for the issue scope

Added more ideas to 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed while I was thinking about it. There is todo in the code for this

Another complication with access is right now as 📌 Save metadata(page data) field with the entity revision Active is done field access is checked in the process of `\Drupal\experience_builder\ClientDataToEntityConverter::convert()`. This make sense as this will be use to set entity fields we shouldn't allow setting field the user does not have access to. But we don't run `ClientDataToEntityConverter::convert()` on auto-save. We couldn't really call `convert()` to determine access though because the EntityConstraintViolationListInterface it returns mixes non-access violations and access violations.

So either we could either

1. extract out something ClientDataToEntityConverter::ensureAcess()` that would be called on autosave, but then access would still be checking in ClientDataToEntityConverter::convert()` during "Publish all"
2. Introduce a new AccessAwareEntityConstraintViolationList extends EntityConstraintViolationList with additional addAccessViolation(ConstraintViolation $v), getAccessViolationsList() and getNonAccessViolationsList() and have `ClientDataToEntityConverter::convert()` return this.

   So then on autosave we could ensure getAccessViolationsList()->count() === 0
   and then on Publish all we could ensure getNonAccessViolationsList()->count() === 0

@larowlan thanks for making this issue.

I think we also might need a temporary generic publish XB changes permissions

The account that submits "Publish All" might not have done the changes to all of the entities(or any?) that would be published. Are we going to require that account to have the permissions to make all the changes across all the entities.

I think having a simple publish XB changes should be good enough for 0.2.0 because once we use Workspaces we want use Workspaces related permissions to determine access to publishing changes to the live site.

Related to 📌 Save metadata(page data) field with the entity revision Active If we went with publish XB changes we would probably want to check access to field changes at the time the changes went into the auto-save. The other option I guess would be to check access as the person who the auto-save considers it's "owner" when submitting "Publish all". The problem with that is that multiple accounts may have edited the same entity. Also the account could have different permissions at that time or been deactivated, hopefully for pre-workspaces, 0.2.0 we won't to worry about these edge case.

But again this may change with Workspace integration so I don't think we should spend that development time on it since 0.2.0 is not for production sites. For example we don't know that once we use Workspaces the final "Publish all" step will still be the point at which we convert auto-saves to real entity saves. We might just use auto-saves at that point to handle: 1) invalid entities, 2) speed or DB bloat concerns from many revisions. We could still require real entity revisions(whether user is aware of them or not tbd) before entities could be move to the live Workspace.

Not completely done but probably could use other eyes on to make this is going the right direction.

Assigning to @wim leers but @larowlan if you happen to review this first and have feedback I can address feel free to assign it back to me and set to Needs work. Obviously also welcome to work on your self.

looking at the openapi related fails in 3494388-force-fail and the fact that 3494388-debug-request-validation doesn't have any openapi fails I think the problem is just requests

So this MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/485 proves at least validation is being triggered for responses.

Opened up https://git.drupalcode.org/project/experience_builder/-/merge_requests/486 to see if the problem is just that requests are not being validated.

📌 Change ApiContentUpdateForDemoController to save from auto-save instead of request data Active should have triggered a request validation error. I can confirm locally it does

re #5 I am pretty sure it is duplicate but @larowlan could correct me. But just to have 1 place to discuss this now let reopen this one. I was really getting confused between the 2 issues

copying my comment from 📌 See why OpenAPI mistake for API error didn't cause test failures Active

@TravisCarden Why not instead of having isProd at all we just leave the other checks in \Drupal\experience_builder\EventSubscriber\ApiMessageValidatorBase::shouldValidate but remove the isProd() check. Then in we could

1. catch the OpenApi validation errors and save the error info in Drupal state under xb_openapi_errors
2. log the errors in \Drupal\experience_builder\EventSubscriber\ApiMessageValidatorBase::logFailure
3. In \Drupal\Tests\BrowserTestBase::tearDown in our base test class(es?) and check the state there and throw an exception. We could have 1 trait for this across all test types

On advantage of this is that you won't fail on the first Open Validation error but could report on all the validation errors in the test run. Another advantage is it won't stop the test so you would also see another failure that would fail the test even if Open Validation failed.

This would also allow say if you were adding a new route locally but were sure on the return structure yet to not do the changes to openapi.yml or not complete the changes until you were sure on the structure, even if you had league/openapi-psr7-validator installed. It wouldn't stop you from working on the test for the new route. Openapi failures would still cause the test to fail but only at the very end so you would know if the rest of the test would pass.

I guess the con would be errors would not stop requests even if you had league/openapi-psr7-validator installed locally during manual testing, only automated tests. But we would also display the errors on the page.

We do something like this in \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::tearDown but in that case we check the logs for specific errors.

I closed 📌 See why OpenAPI mistake for API error didn't cause test failures Active and pushed the MR there to here

closing and pushed this branch to the other issue

Actually can we close this issue and just work in 📌 Openapi validation broken in HEAD Active . Would be easier to follow. You could multiple MRs in the other issue

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

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

Thanks @larowlan and @effulgentsia

Thanks @wim leers and @larowlan!

Asking for @wim leers review again. The test cases are being moved to

1. \Drupal\Tests\experience_builder\Kernel\ClientServerConversionTraitTest::testConvertClientToServerErrors
2. or \Drupal\Tests\experience_builder\Kernel\ClientDataToEntityConverterTest::testConvert if the errors are caught in \Drupal\experience_builder\Controller\ClientServerConversionTrait::convertClientToServer but only caught in \Drupal\experience_builder\ClientDataToEntityConverter::convert

\Drupal\Tests\experience_builder\Functional\ApiContentUpdateForDemoControllerTest::testSave is still left with test cases that prove that if the call to
$violations = $this->clientDataToEntityConverter->convert(json_decode($request->getContent(), TRUE), $entity);

gets an error entity will not be saved

It would be great if we could get a couple issues in first that would make this issue smaller.

1. 📌 Change ApiContentUpdateForDemoController to save from auto-save instead of request data Active : If this was taking directly from the auto-save then ApiContentUpdateForDemoControllerTest would not have to be updated to change it's call to \Drupal\experience_builder\ClientDataToEntityConverter::convert to deal with metadata and the client would not have send the meta fields.
2. 📌 Move logic out of ApiContentUpdateForDemoControllerTest into more other tests Active : These test cases will need to also incorporate metadata and should work regards of the controllers that use them

tedbow → created an issue.

The only drawback of this method is that "Publish" will not work with a complete empty canvas. But since this is temporary button I think that is fine.

Not a blocker but I think we should also do this 📌 Change ApiContentUpdateForDemoController to save from auto-save instead of request data Active . who knows how long the ApiContentUpdateForDemoController will actually be around and it should be doing the save in the same way general way, ApiPublishAllController is, including for metadata fields. That issue is very small change

tedbow → created an issue.

The 1 e2e test that failed `contextual-panel.cy.js`, is now passing on re-run. I think I answered @wim leers question in #12 but since it is just the difference of changing a comment we can always open an another MR on this issue to fix.

Not sure if this issue is actually going to tackle of the problem of making the request to the server to get the pending changes but server now supports this see 📌 Create AutoSave service and HTTP API to retrieve all entities with pending changes Active

If this issue is not taking that on is there a follow-up that will?

the logic can be seen in the phpunit tes.

the client first has to make a call to the changes see \Drupal\Tests\experience_builder\Kernel\ApiPublishAllControllerTest::getAutoSaveStatesFromServer
and then send those changes back to the server(to confirm it knows about the latest) - see \Drupal\Tests\experience_builder\Kernel\ApiPublishAllControllerTest::makePublishAllRequest

Nice, thanks @bnjmnm!

created follow-up 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active

To me this is not a top priority because it is not needed for 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active but I think would good others wanted to take it on

tedbow → created an issue.

I tested terms and it doesn't seem to work to publish

Personally I think we should leave the restriction in for now and open a follow-up to add test coverage.

I think this issue is good to get in because it unblocks the Page work but I don't we should hastily assume everything work just because we solved the field name problem

Re #12

I was going to say I had tested this and didn't work with Basic Page but now I tried one more time and it actually does.

Interesting the problem I saw before was that the XB edit work but then the "Publish" button only worked, the changes actually showing on viewing the node, only worked for Articles. Not sure

But I think if want remove this restriction I think we should add a test in this issue that uses with another entity type to make sure everything works correct, not just rely on the manual testing.

Interestingly there are no e2e test for publishing because know was added in ✨ Connect the "Publish" button with the entity update controller Active . So do we just remove the article restriction now when all of our tests use article?

actually I going to fix a couple things

only empty-canvas.cy.js is failing which is known issue . see 📌 Remove leftover wait() in empty canvas e2e test Active

The only place I see left where have hard-coded articles besides setting up the field_xb_demo field is in
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent::getClientSideInfo:

$suggestions = $this->fieldForComponentSuggester->suggest($component_plugin->getPluginId(), EntityDataDefinition::create('node', 'article'));</code>

But I don't think is a quick fix because there is no $entity there to remove the hard-coded `article`. My guess is that practically won't have an effect because field.value.component_tree doesn't all dynamic under ComponentTreeMeetRequirements so won't get fields that based on entity definition anyways but it has been a while since I have looked at that so I am not positive. I will create an issue for that

changing status and priority back because of reasons above. chatted with @bnjmnm and those changes in #7 weren't intentional

also a suggestion. Should we make a separate quick MR to either mark this test as skipped(if possible) or remove it completely? it would be easy to add back. As of now we are committing issues with this failing, which I think it is fine. But if you weren't in the know I could see someone wasting time on another issue trying to see if there changes somehow cause the failure.

Just don't want people wasting time to figure this out.

Assigning @larowlan because he made the change in 📌 Create AutoSave service and HTTP API to retrieve all entities with pending changes Active which I removed because it was out of scope

working again on this

A couple suggestions but I think this looks pretty good

Reviewing but see uses of `field_xb_demo` in src/ClientDataToEntityConverter.php for the error messages

Looks good!

reviewing

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

Done! thanks @larowlan and everyone!

Test are passing except for cspell which is know issue fixed in 🐛 Fix cspell errors Active we didn't affect that file

empty-canvas.cy.js is failing but that is known issue being worked on in 📌 Remove leftover wait() in empty canvas e2e test Active

Merged because only change was to dictionary.txt and cspell test passed

@longwave thanks

RTBC, contingent on tests passing

tedbow → created an issue.

If 11.1.x passes I would suggest we use that and then leave this issue as critical. Only so many people can probably work on figuring this out and meanwhile it would good if other issues could get committed. Also maybe 11.x will be receiving a lot of commits while Drupalcon Singapore is going on?

Setting to "Needs Work" as there is active MR

@jessebaker and @bnjmnm thanks for working on this!

@traviscarden thanks for the review !

Test are still failing because of 🐛 PHPUnit Next Major tests failing Active and 📌 Remove leftover wait() in empty canvas e2e test Active (e2e)

Yep still failing

Looks like it is failing for different reason https://git.drupalcode.org/project/experience_builder/-/pipelines/364038

I triggered the jobs to re-run on the same pipeline so I am not sure we can see the previous results

Seeing if I can re-run the pipeline here with a new commit

re #12

landed so this should no longer be necessary - @tedbow can you confirm?

re-running tests on 0.x. https://git.drupalcode.org/project/experience_builder/-/pipelines/364038

there is e2e test that also appears to be failing but that is unrelated. see 📌 Remove leftover wait() in empty canvas e2e test Active

I think this is critical because I am not sure if we want to commit other issues since all test won't pass. Not sure how strict we are on that

assigned to @traviscarden for openapi.yml approval/review

Created follow-up 🐛 ui/tests/support/commands.js should set 'host' = '/' Active

tedbow → created an issue.

I think this issue is done but there are couple problem in 0.x that makes test fail.

📌 Remove leftover wait() in empty canvas e2e test Active causes e2e tests to fail.

🐛 PHPUnit Next Major tests failing Active found an error that should be fixed in the core issue 📌 Install modules with container_rebuild_true set by themselves Active

Assigning to @wim leers because he needs to .gitlab-ci.yml changes

@longwave pointed out that 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active was only added to 11.x. The current just switches our phpunit (next major) to use 11.1.x. Assuming this passes test I suggest we commit this and leave the issue to remove the need for the override

tedbow → created an issue. See original summary → .

RTBC'ing based of @f.mazeikis approval

Chatted with @wim leers and created an issue to extract out just the changes for ClientServerConversionTrait and ClientDataToEntityConverter 📌 Provide a utility method to convert client layout and model to server tree and props Active

The changes under /src were already reviewed by @wim leers in 📌 Provide a utility method to convert client layout and model to server tree and props Active . the the new function \Drupal\experience_builder\Controller\ClientServerConversionTrait::convertClientToServer is mostly just extracted from \Drupal\experience_builder\ClientDataToEntityConverter::convert.

I added a test for the new function \Drupal\experience_builder\Controller\ClientServerConversionTrait::convertClientToServer but this also covered indirectly because it is used ClientDataToEntityConverter::convert which is gets invoked in ApiPublishAllController and ApiContentUpdateForDemoController so it is covered in their tests.

keeping assigned to myself. I am actively working on this

tedbow → created an issue.

Not currently working on this

going to work on this

I talked with @f.mazeikis about this issue today

I wanted to see if the logic we added in ClientDataToEntityConverter could be used in this case also. When added in [##3489994] it hardcoded assumptions that we would be updating an entity with a ComponentTreeItem which of course in sections we won't be. But we did have most of the logic for converting a `layout` and `model` from the client to a `tree` and `props` that server can save.

So I moved most of this logic to a new ClientServerConversionTrait::convertClientToServer. These leaves both ClientDataToEntityConverter::convert and ApiConfigControllers::decode very thin as they can both rely on the the new convertClientToServer

I talked with @f.mazeikis about we need to figure out if we can centralize the conversion from client to the server and then back. But this at least puts the logic for client to server in 1 place for converting a `layout` and `model` from the client to a `tree` and `props`. there really wasn't any new code in here just moving code around that already went through review

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

First want to say I am so glad Package Manager is in core so we can get more eyes on it to make improvements.

Theoretically we might even want this to be this way in the actual package manager module?

Just want to say that we did the audit in package manager because we were a bit paranoid that our test setup can only test so much and there might be many many differences among real composer projects. So we thought it safest to use audit to hopefully at least ensure the projects are the way composer expects or would want things set up.

I think running composer operations on live site, even though we do it in sandbox first, is potentially dangerous so I think being paranoid and trying to ensure setting up things the *right* way, as much as possible, is warranted.

Thanks to @dries and the core committers for getting this discussion going and creating the doc. I really like the change of focus.

Here some thoughts based on the PDF

Drupal Core is a platform that empowers both low-code users and experienced developers to create powerful, user-friendly CMS solutions.

Can we say that certain audiences we don't want to go after? Which ones? Users who aren't comfortable with "low-code"? Meaning you will have to be okay with some code and config? It seems implied in this idoc but not said anywhere the Drupal Core is not really for *everyone* if you have no experience with some web development this is probably not for you. Maybe you start with Drupal CMS? I think this is a good thing to narrow down our audience.

Any organization that already has sites built on Drupal core and are maintaining them long term,

So does this mean Drupal core still has to prioritize these sites even if they don't fall under the current area of where we want to "play"? Does this effectively mean we can’t narrow where we play?

Experienced Drupal developers and site builders who are comfortable hand-picking modules and recipes to build out a site, but are not necessarily going to create their own repeatable CMS for this purpose

(emphasis mine)
Can we narrow the focus to "complex" sites, otherwise it seems to negate the narrowing above to "complex or repeatable"? Drupal core probably isn’t for experienced developers unless they are building a complex site, otherwise there are better solutions.

Where will Drupal Core play?

This is a long list. It would great to hear explicitly where we are not going to play or at least what is the most important in this list. Otherwise it feels like the "playing everywhere" problem mentioned in the video.

Establish channels for faster feedback loops between developers, site builders, and end-users

Which end users? The end users of Drupal core? Or the site builders and the end users of the CMS's built on top of Drupal core?

Ok started a solution to #13 will comment on the MR

I talked to @effulgentsia about this issue and 📌 Create an endpoint to publish all auto-saved entities Active which together will allow the UI to create "Publish all" button

I think we missed something between these 2 issues

Here is the UI proposed workflow

1. User clicks "Ready to publish"
2. User sees a list of all entities in the auto-save state that are ready to be published and the dates they were last changed
3. User clicks "Confirms"
4. if auto-save states matches what the user just saw the all the changes will be published. This means there can't be any new changes since the user saw the list. No new entities, no entities missing, no new changes to the entities
5. if there are changes since the user saw the list they will be prompted to confirm after seeing the new list.

So as it is now ApiPublishAllController doesn't take any input from the client but we probably need to change that where the client sends back some info that it received from ApiAutoSaveController. Otherwise the user could see a list of changes to be published wait an hour while other changes are made, without them being aware, click "publish all" and then they have published things they are not away of.

comitted 📌 Create an endpoint to publish all auto-saved entities Active

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

@traviscarden also made a follow-up 🐛 Add missing response to `/xb/api/publish-all` in `openapi.yml` Active

🤔 MR button is gone for me too!

@larowlan and @lauriii thanks for finishing this off!

One thing, I noticed is that we have todo to remove ApiContentUpdateForDemoController which also means in that issue we will probably also remove ApiContentUpdateForDemoControllerTest.

The problem is that ApiContentUpdateForDemoControllerTest covers a lot of the logic that is now in ClientDataToEntityConverter. We have the new ApiPublishAllControllerTest but that doesn't cover all the cases we cover in <code>ApiContentUpdateForDemoControllerTest

for instance the logic around creating 'pointer' => "layout.children[1]", but I am pretty sure there is more.

so we can't just remove ApiContentUpdateForDemoControllerTest when we remove ApiContentUpdateForDemoController or we lose our indirect test coverage for ClientDataToEntityConverter.

So we should probably make a follow-up to create ClientDataToEntityConverterTest and move most of the error test cases from ApiContentUpdateForDemoControllerTest into that leaving ApiContentUpdateForDemoControllerTest to be just a small test to cover the logic in the controller that can be removed when we remove the controller.

Changes since my last review look good. I think this is basically ready. Will review again after 📌 Create an endpoint to publish all auto-saved entities Active is and we can remove those changes

Assigning back to @wim leers , re #10 I think this is unrelated issue and pushed up a change that removes a need for the changes to `XBPreviewRenderer`

I think the rest of @wim leers comments on the MR are about changes to the files that are no longer needed