🇺🇸United States @tedbow

🎉 Thanks @wimleers and @penyaskito

Playwright finally passed by now I had to merge 1.x, 🤞🏻

MR 197 is passing except playwrite, which always fails for me so I don't know how to determine if it is ok and a couple e2e tests. Could retrigger to see if e2e tests pass

so it looks like in actually brings in 11.2.3 if we don't specify

See https://git.drupalcode.org/project/canvas/-/jobs/6813269/artifacts/file/...

{
            "name": "drupal/core",
            "version": "11.2.3",

I added one last commit to reorder the requests in testDynamicProps so that the 2 valid requests were together.

I also manually tested linking an empty required field to a required property. I did this by adding the required field after I made the preview node. It worked as expected, able to link the field but the component did not render because it was missing a required property

@wim leers let me know if you want me to add test case for this. I think it follows the same code path as the non-required field so I don't think it is necessary

@wimleers thanks for the review

re #16, I think I implemented all your suggestions.

I know you approved the MR but since introduced `\Drupal\Tests\canvas\Kernel\ComponentInstanceFormTest::getFormCanvasPropsForComponent` which is pretty big changes, though I think it is good refactor

tedbow → created an issue.

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

I tried to manually tests this. I merged this with 1.x but I couldn't find a component that I could link a prop to this field. Is there an existing one? I enabled `sdc_test_all_props` but it didn't have a prop I could find either. Since this mainly for content templates it seems reasonable we should be able to manually test this before merging

Also is there a reason not to create an adapter instead of base field. Won't a base field have the side effect of it being able to used in other module UI's but an adapter won't?

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

re #12. I solved it in the test. See comment about using canvas_stark test and PR comments

I have set this issue to Needs to Review.

It is still having 1 php warning triggered by this:

$form[$sdc_prop_name]['widget']['#prop_link_data'] = [
            'linked' => FALSE,
            'prop_name' => $form[$sdc_prop_name]['widget']['#field_name'],
            'suggestions' => $suggestions[$sdc_prop_name],
];

I put a note in the MR itself as why this triggers a warning. I can't reproduce outside of the test

If anyone knows how to quickly fix this that would be great. Otherwise I think it would be good to get this sense it is marked critical and make a follow-up to solve the php warning issue

1 test done, 1 to go

I think we need 2 tests

1. \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\SingleDirectoryComponentTest, we already have `testInputToClientModel` but now testClientModelToInput. We could test with one component with both static, dynamic non-empty, dynamic empty
2. ComponentInstanceFormTest

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

@wim leers, merged(or currently merging)! see my comment about my 1 additional change under src to get a test passing https://git.drupalcode.org/project/canvas/-/merge_requests/150#note_598442

There still needs to be fixed. So far it has just been adding permission to test cases where they are now needed.

Others could fix the remaining tests, or I will take a look at them tomorrow and re-assign to myself.

Right the functionality may be good for initial first take at least making sure we are not disclosing data the user should have access to

1. When rendering a component with any dynamic prop source you don't have access to you see "Component failed to render, check logs for more detail.". This is try in the preview and live(once content template is published)
2. The error is logged so you with information about which component and expression
3. When trying to set a prop to a dynamic source you don't have access to the setting will not succeed and nothing changes. This because the PATCH fails. There is an error reported to the client but you can only see in the js console. We could open a follow-up for the client to report this and other errors when trying to set a prop. I think this better than the current situation where you can set the prop and see data you shouldn't see. So I don't think it is a regression
4. If a Canvas prop is already set by another user to dynamic source the current Canvas editor does not have access the form fails to load.
   with this error
   

   this breaks the front end UI. After I get this I can't load any of the other component forms even though they don't have linked dynamic sources. It appears the client is not making any more requests for the forms. I think the client should handle this more gracefully, to allow other forms to work after this

I started an MR to just throw exceptions

In the chat I mention in #11 we thought if user was working in canvas they should be able to undo setting if they get the exception if it does not renderer.

I tested with changing the "content editor" role in standard to have all the permission under "canvas" no other additional changes..

If they try to link to "Authored by → User → name" and they are the author of the preview entity it works fine. If they are not the author then it just appears in the UI that linking didn't work and the linked value is still the static source. In the console you can see there is 403 on the patch request.

If another user has set the value to "Authored by → User → name" and the editor then tries to edit the template they get an "An unexpected error has occurred while fetching the layout. Error 403: Access denied to entity while evaluating expression iFsentity:user&namersusvalue" modal error and they can't access the UI to fix it.

I updated the title/summary based chats with @lauriii @wim and others

I will work on an MR that just throws and exception if the user does not access to the linked field value

I had suggested we hardcode removing revision_log from linkable field suggestions because we were only supporting node templates at first. I thought this would mean you could NOT get into this situation with just core, since few fields have view access restrictions and core does not offer a UI for field level permissions. But I forgot we allow linking to references, i.e the author reference and its fields. Also you could create other references to different content entity types. So there are probably at least a few ways to get into this situation with just core

Coming back to look at this I think we need clarity on what the product requirement is

1. re

   Right now, the ContentTemplate::ADMIN_PERMISSION is assumed to be sufficient: if you have this, Canvas assumes that you have access to all fields. Because if not, how could you ever properly edit a ContentTemplate?

   I think there would a lot of cases where you would not want to allow a person design content templates that does not have access view all the fields. Especially since we are promoting Canvas as a way for "site builders without Drupal experience to easily theme and build their entire website using only their browser, " → . This with the fact that we are offering a way to stage template changes before publishing them means many sites will probably be creating this config changes on production without santatized data.

   People often use Drupal fields to store internal data they never intend to show to the general public or even every member of the their organization.

   Imagine a product content type that has "Internal product note" with the value "this product will be discontinued next and will not offer support to our customers". Or a "team member" content type that has "contact info" or "hr note" fields. These types of fields now might only be viewable on the edit form or they might be rendered to users with view access but their appearance is probably not as important as fields customers are going to see.

   I would think you would want to be assign someone to make content templates without worrying they could simply briefly use the field as dynamic prop source just to see the values of fields they shouldn't have access too. Since return back all matching field suggestions this would be very easy to figure.

2. The other question is what to do if the end user does not have access all of the fields used for dynamic fields. If the user doesn't have access to field linked to a required property it seems reasonable to just not render the component. but what if they just don't have access to a field linked to optional property? We could still render the component but with optional property empty.

I think the immediate question is what is needed for beta

Ok. added a much longer code comment as to why sorting happens they way it does.

I think the solution is correct now that I understand better how `\Drupal\Component\Graph` works. Though I think our needs are a little different then other cases where Graph might be used. In our case we need to keep the top level items in exactly the other they were sent in relative to the other top level items. So we can't rely on 'weight' set by \Drupal\Component\Graph\Graph::searchAndSort to be the order we want to top levels items when returned from \Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemListInstantiatorTrait::generateComponentTreeKeys. We always want those to be same as the input top level items.

I think that is the last MR we needed on this issue 🎉

I think if we find another bugs etc, we should create new issue

Re #50 I do see the re-ordering problem in this branch but I also see it in 1.x. I don't think it caused by this MR but by 🐛 Components in content templates are not always rendered in correct order Active

If apply those changes(which are different file) the ordering bug does not happen in 1.x or this MR

From #50

with the correct Layout/Model JSON (you can see the order has changed) but no new autosave is generated (note no changes button in the top right doesn’t update) and the preview is refreshing but the markup doesn’t reflect the change.

Yes this makes sense even though the correct layout is being sent from the client the bug in 🐛 Components in content templates are not always rendered in correct order Active which is in \Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemListInstantiatorTrait::generateComponentTreeKeys causes the layout changes to be the order to be the same as the published order. But this just happens to be the same in this case. whenever the layout/model goes back to the published version the "no changes" goes away because delete the auto--save entry or don't create one. That is by design

But if you do a re-order where 2 column component you are moving a heading into is not already at the top, the bug will happen, the two column component is moved to the top, and the auto-save happens. Because output after the bug is still different from the published version

See this example

You can see the when move the heading into the column the re-order bug happens and "review 1 change" shows up.
You can also see at the end when I manually move the components back to published order the "no changes" shows back up

Changing the title because this effects regions also. This would affect patterns also but since you can't edit them I guess it won't?

Add the test cases from figuring out the actual values sent from the client.

We should add a case to `\Drupal\Tests\canvas\Kernel\Config\ConfigWithComponentTreeTestBase::testComponentTreeKeyOrder` simulates what would have been failing test case

I was manualing test a case I got from @jessebaker

so I had
hero
hero
two column
heading
and as soon as I put the heading into the two column slot the order changed to
two column
heading
hero
hero

re #2 I am pretty sure, that it is still broken in that MR. At least when I test.

From my testing this also happens in Page regions and is coming from \Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemListInstantiatorTrait::generateComponentTreeKeys

which would make sense because this is suppose to order the items. That would make sense why it does not happen in Page entities

tedbow → created an issue.

Its down to phpstan error I am ignoring 🤷

Ok went through explanation @wim leers gave here and look at the relevant code, makes sense

@wim leers yep sorry I missed what was really going on here.

Sorry missed the default config phpunit test fails. Probably easy fix

The one issue in #6 has been addressed and we have a screenshot

I reviewed this and step through the code with and without the fix. this works!

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

Since \Drupal\canvas\Controller\ApiLayoutController::updateEntityFromClientData was already updated in the other MR in this issue I didn't to have to make any non-test changes for this last MR which turned out to be the case. Now updateEntityFromClientData() is only used in POST because PATCH update the server saved entity have updating the inputs from the convert model

TLDR,

I don't we should be creating a media entity with a placeholder image since every SDC with an image prop.
I think either

1. We should just always just the default value for the image prop
2. If there is a media image we should use it, otherwise use the default value for the image prop

I would vote for 1 because it is the simplest. With 2 you still have to make a function to tell the AI how to get the default value for the image prop aways. So we could do number 1 and then do number 2 if it it not good enough.

More in depth:

I looked into it more and pushed up some changes to `\Drupal\canvas_ai\AiResponseValidator::convertToComponentTreeData` to convert the client format of the props to the server format via \Drupal\canvas\ComponentSource\ComponentSourceInterface::clientModelToInput. This is needed so that \Drupal\Core\TypedData\TypedData::validate works.

For may components I think `clientModelToInput()` might not make any changes but for some, for sure ones with images it will.

Re my comment on the MR to not just make media entity with a placeholder if the site doesn't have any, I think the solution is instead to use the SDC default value for the `image` prop` if there are no media images available.

with the changes I was able to validation to pass at '/admin/config/ai/explorers/tools_explorer?tool=canvas_ai%3Aset_component_structure' for this structure

reference_nodepath: [0, 0]
placement: above
components:
  - sdc.canvas_test_sdc.card:
      props:
        heading: "Delicious Cookie Selection"
        content: "Indulge your sweet tooth with our mouthwatering variety of freshly baked cookies! Choose from chocolate chip, oatmeal raisin, snickerdoodle, and more—all made in-house every morning."
        footer: "Order today for same-day pickup or delivery!"
        loading: eager
        sizes: "auto 50vw"
        image:
          src: /modules/contrib/experience_builder/tests/modules/canvas_test_sdc/components/card/balloons.png
          alt: 'Hot air balloons'
          width: 640
          height: 427

Notice `/modules/contrib/experience_builder/tests/modules/canvas_test_sdc/components/card/balloons.png` is not directly the example in `tests/modules/canvas_test_sdc/components/card/card.component.yml` but the path based on the where the module providing the component is.

If you goto to /canvas/api/v0/config/component you will see `/modules/contrib/experience_builder/tests/modules/canvas_test_sdc/components/card/balloons.png` as the default value for the prop.

You could should be able to find the default value like this

$component = Component::load($componentId);
$clientNormalized = $component->normalizeForClientSide()->values;
$default_value = $clientNormalized["propSources"]["image"]["default_values"]["resolved"];

I think the initial problem of why the validation failed in \Drupal\canvas_ai\AiResponseValidator::validateComponentStructure is because \Drupal\canvas_ai\AiResponseValidator::convertToComponentTreeData actually doesn't set the component inputs correctly. For many property there does not need to be a transformation of the values from the client to server model but images there does. Looking into that

I am wondering what the expected functionality should be for required fields that user does not have access to view if they used for required properties

The field suggester I think intentionally only matches required Component properties to require entity properties so a component won't be try to render without a require property
Should a field that user does not access to act the same way as a require field that is actually empty?

I tested

1. I added a text field that was not required
2. added a basic page without that field.
3. Made that field required
4. Used ✨ Allow linking a component prop of a template to a dynamic field Active to link the field to the Hero header, a require property
5. got a component failed to render error when using that node as the preview

The example is convoluted but could happen.

I think this is similar to a field that is required but some users can't view it. The component instance would let the admin link this to required field.

Seems like it would be pretty common as sites might have required fields that are never meant for site visitors but are using for internal purposes.

tedbow → created an issue.

@mayur-sose to test this issue

1. Add a Basic Page(not a canvas page) with title "my page"(/node/add/page)
2. In a new tab go to /canvas
3. Add a page so that menu shows up on the left(I am not sure if there is different way to get the menu)
4. Click the "Templates" icon on the far left(bottom most icon)
5. click "add new template"
6. in the pop up select Content Type: Basic page, Template name: full content, click button "add new template"
7. You should see a canvas page with the title "my page", this is the Content Template with the title of the preview content 🎉
8. got back to tab with your Basic page
9. edit the Basic page
10. changed the title to "my new title". save
11. go back the canvas content template page
12. refresh browser tab
13. The title should now show "my new title"

😱
I have gone and pushed the "radica l refactor" MR, sorry/you're welcome, I don't which it should be😂. It is only a refactor of `ApiLayoutController` to be clear

This MR does not change test coverage at all and it still passes, just phpstan and think random e2e and playwright fails

Basically there was a lot of logic ApiLayoutController that I think we only have because we use store client data directly in auto-save and now we store the converted entities.

Whether we make these changes in this issue or a follow-up I think we should address this

@wim leers, since we went did kind of big refactor of ApiLayoutController to try make things clearer, I think it also fixed some existing code the confusing. See this for my reasoning https://git.drupalcode.org/project/canvas/-/merge_requests/77#note_589161
I left a couple other notes on that MR but that is the main reasoning

The MR is green except for 1 cypress test, which I am just rerunning. But If you were hoping to just wrap up this issue(at the PATCH portion) we can also just revert back your last commit and cleanup/commit.

I personally the refactor code makes things clearer but it could wait

😬Also......

I didn't really like the solution of the having to update the entity from the client data and then
setting $data['model'][$componentInstanceUuid]['resolved'] = $component_model['resolved'];. I think refactoring helps but it still feels a bit weird.

So made yet another MR, split from the above MR here. This is just running CI and am not confident it is going pass(phpstan definitely not). But the tests I ran locally pass. So this probably for another issue to take if we want to. But it does not solve the problem of having to set `$data['model'][$componentInstanceUuid]['resolved'] ` directly.

If gets around this by setting the input for the item directly from the PATCH data before updating entity directly after retrieving auto-save(if auto-saved). Only then does it auto-save the new data nad convert the entity and regions to the client format.

Again this new MR is probably only a thought experiment right now

I also noted that if I tried to the link the "revision log" to the sub heading this did not work. I think it might be because the revision log was empty. If I updated the revision log so it was not empty it worked

Just tested ✨ Allow linking a component prop of a template to a dynamic field Active is affected by problems found and fixed in https://git.drupalcode.org/project/canvas/-/merge_requests/77

I test this manually

I was able to link the heading to the node title but if I tried to link another field I got a server error. It was fixed by an MR am working on here https://git.drupalcode.org/project/canvas/-/merge_requests/77
So we should get the MR in soon and continue on working on other issues in here 📌 [PP-2] Accept Dynamic Props in ApiTemplateLayoutController for content templates Postponed

tedbow → created an issue.

Leaving assigned to myself.

In the new MR I have https://git.drupalcode.org/project/canvas/-/merge_requests/77

1. I have updated changes to allow `\Drupal\canvas\Controller\ApiLayoutController::patch` not fail if there existing dynamic props source, whether there in the component being updated or not
2. Ensured that "resolved" will be returned with the correct dynamic values for the dynamic prop sources.
3. Expanded test coverage in \Drupal\canvas\Controller\ApiLayoutController::patch for the points above

We could merge this MR with just PATCH fixes and test coverage if this unblocks the front-end work and would allow setting and rendering of dynamic sources in conjunction with ✨ Allow linking a component prop of a template to a dynamic field Active

The other option would be in this MR to add test coverage(and any fixes it needs) for POST also

I would vote for getting current MR with just PATCH fixes as it might point out problems with #3541037

Either way I will work on this tomorrow. I have not had time for a self-review, which I will do tomorrow. but @wim leers feel to review whats here or take it over if you want.

@wim leers that is pretty much correct.

My understanding is the client won't actually use the values under `resolved` returned from a PATCH request. but since the client doesn't support setting dynamic sources we can't test it.

you'd just need to trigger a reload in the browser for this to start working in Canvas.

I think the client would already update the preview on PATCH request but I could be mistaken. If the client does not update the preview on PATCH request is it just not using the `html` key that we return back? if it does use the `html` key we return back to update the preview this should already work when support setting dynamic props because the new test proves the `html` does reflect the dynamic prop sources.

I will need to check with front-end folks to confirm how this works

I did run into another problem after I merged this that a PATCH request would error if there existing dynamic sources used before the PATCH request. I have created another MR for this https://git.drupalcode.org/project/canvas/-/merge_requests/77, basically \Drupal\canvas\Controller\ApiLayoutController::getLastStoredData which is only used in PATCH was not taking into account the preview entity so it would get the "missing host entity" exception

Thanks for closing this @effulgentsia

re #23 merged and set to needs work for another MR

@wimleers you will probably want to confirm the last merged MR

for https://git.drupalcode.org/project/canvas/-/merge_requests/70

I think the new MR is ready for review/commit https://git.drupalcode.org/project/canvas/-/merge_requests/70

@wim leers approved these changes yesterday the MR shares the same history as https://git.drupalcode.org/project/canvas/-/merge_requests/58 up to the appoint approved, which in the new MR is https://git.drupalcode.org/project/canvas/-/merge_requests/70/diffs?comm...

After I just added 1 change in \Drupal\canvas\Controller\ApiLayoutController::buildPreviewRenderable to pass $preview_entity to \Drupal\canvas\Controller\ClientServerConversionTrait::convertClientToServer. This ensures the dynamic properties can be resolved. This is will have no effect on non-Content Template entities because $preview_entity will always be NULL.

I then added test coverage to prove you can update props to dynamic sources using PATCH and they will be reflected in the `html` returned in the subsequent GET requests.

There is a todo to make sure `resolved` is correct in PATCH requests for dynamic sources but it already is for the next GET request. We can tackle that in another MR in this issue where we will also add test coverage for POST requests.

@wim leers, the patch logic turned out to be tricker than I thought. Especially if we want the client not to have to send back "resolved" for dynamic components, which it can't resolve and then also to have "resolved" be return correctly for patch requests.

I have some sort of solution but it is very rough as I just was able to finish this at my EOD, so also not explained well probably.

I put comments in the code if you are interested but also feel free to wait until I can explain my reasoning better.

Leaving assigned to myself

tedbow → created an issue.

forgot test coverage for POST and PATCH

I couldn't get https://git.drupalcode.org/project/canvas/-/merge_requests/51#note_585851 to pass phpstan expect by ignoring the 2 times we call setComponentTree with collasped inputs like

       'inputs' => [
          'heading' => 'Hello, world!',
        ],

instead of the expanded array for the input
Before we were using ->set('component_tree', ....) so there was no phpstan validation happening for the array.

Maybe someone with better phpstan skills could fix it.
We could commit what we have as it gets back to where were before this issue, which I believe next validated the inputs array for collapsed inputs

good find!

I think the approach in MR 58 works.

I am going to try different in different MR

tedbow → changed the visibility of the branch 3541057-render-dynamic to hidden.

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

🎉 Thanks @wim leers

Chatted with @wim leers.
To avoid making issue take longer I reverted
https://git.drupalcode.org/project/canvas/-/merge_requests/31/diffs?comm... and https://git.drupalcode.org/project/canvas/-/merge_requests/31/diffs?comm...

Wim had tried to make sure we were using ContribStrictConfigSchemaTestTrait but that needed changes to \Drupal\Tests\canvas\TestSite\CanvasTestSetup which then caused e2e test failures

We already have 📌 Decouple kernel tests from XbTestSetup Active . I commented on that issue to try and use ContribStrictConfigSchemaTestTrait

In 📌 Refactor(or attempt to) all or most ApiLayoutController*Test classes to also test ContentTemplate entities Active we tried to use ContribStrictConfigSchemaTestTrait but we couldn't because of this issue. So it would be good to add that if we can in this issue

Re #7 I think all the tests that need to be converted have actually been converted now. So unless we find something else I don't think we will need to make another MR on this issue

Created follow-up 📌 Add test setComponentTreeValue to ComponentTreeEntityInterface Active and added todo

tedbow → created an issue. See original summary → .

I think all the tests should pass now, re #7, I think we should merge this ASAP and then continue working on the rest of the tests

I have made some progress,

tests/src/Kernel/ApiLayoutController[Get|Post|Patch]Test.php now have test coverage for content templates too.

In process I found and fixed some bugs that we have hit because our current limited test coverage.

Right now a couple test I have edited are failing, haven't investigated why yet.

Because the Content Template UI work will probably run into the bugs either when manually testing or writing tests I think the next steps here should be

1. Fix the current test fails
2. Clean up the current changes
3. Merged the current MR
4. Open another MR to continue on updating other tests.

There is bug where suggestedPreviewEntityId is being sent back as a string not int.

Will post fix and test in new MR

phpunit test failed

Got new MR https://git.drupalcode.org/project/canvas/-/merge_requests/10 almost passing

assigning it back to @wimleers because @f.mazeikis had assigned to him

currently creating another MR for canvas

Assigning to myself to create a new MR against "canvas"

@wimleers see my last MR comment

@thoward216 and @wimlees a got further in the test but the relieved some other, problems, not with your code, but some things I hadn't thought of.

Working on it