🇺🇸United States @tedbow

tedbow → created an issue.

tedbow → created an issue.

Full disclosure, I found a few things and then asked used claude to find similar problems

tedbow → created an issue.

tedbow → created an issue.

@marcus_johansson #6, yep thats correct

Added other notes in the MR

Is the idea that if it catched it once, you don't have to switch over the provider as well? I think that would be fine, just clarifying.

Yeah that idea would be if you checked the new settings

1. LogMockRequests would still record request but they would all be enable
2. For all providers(except Echo provider): it will check if there any matching request and use them if so

So basically don't have to switch providers. It probably makes sense move that logic out of EchoProvider, because would be in effect without EchoProvider being selected

Setting to Needs Review because I want to make sure the maintainers are interested in adding this in general before I work any more on it

tedbow → created an issue.

tedbow → created an issue.

There are no current e2e test. Tests would have already failed because I think tests run in sub-directory

tedbow → created an issue.

tedbow → created an issue.

tedbow → created an issue.

But I think I don't actually understand how the tags are being used write now. They show up in the UI but as far as I can tell they aren't actually used for anything, correct?

If they are currently not used for anything would it make sense just to remove this until there is a concrete use case?

I think the problem with a just a plain multiple value field is that it is doesn't give you auto-complete for already created values. Though you could make a field widget that helped with that.

What about providing a very simple config type, AI Context Tag(or better name), ai_context_tag. Then you could still use the `entity_autocomplete` widget. You wouldn't have problem of different ids on production

You could still have the name collision but that is true for any config entities, and ai_context entities themselves

i ran into this try to install the module.

@roromedia fix looks right

@the_g_bomb thanks for reporting back!

Added follow-up 🐛 Handle config component being delete gracefully Active for #42.TC03

tedbow → created an issue.

@the_g_bomb

I'll try again locally with rc1, apologies if this is a false report and if needed,

No problem. And thanks for the added info on how you are testing.

I tried https://www.drupalforge.org/template/drupal-canvas and I can't reproduce the problem.

I wonder if it could be Javascript problem that is browser specific. I am using Chrome Version 141.0.7390.66 (Official Build) (arm64), on a Mac

What are you using?

tedbow → created an issue.

Looks good!

I am re-running Playwright tests, hopefully random

I can't reproduce this with 1.x or Canvas 1.0.0-rc1, when using it directly and not with Drupal CMS. The title field never loses focus, even though I can confirm the page gets auto-saved.

@the_g_bomb can you confirm what version of Drupal Canvas you site has installed. composer info drupal/canvas should tell you. Testing just with Drupal canvas should tell if this is problem only in Drupal CMS or with Canvas itself

Probably makes sense but because it was never on the Page entity it is not a bug

@mayur-sose,

I used https://github.com/phenaproxima/canvas-demo

When I submitted the form I got the error you documented in #23 when submitting the webform

But then I ran composer require drupal/canvas:1.0.0-rc1

This ran Upgrading drupal/canvas (1.0.0-beta2 => 1.0.0-rc1)

AFter that I no longer get the error

Everything looks good. Just a 1 e2e test failure, playwright failures. guessing random

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

FYI the merge file tracking-required-fixture.php has phpcs error

There one e2e test failing but it is global-regions-interact.cy.js and that test does not interact this page

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

oK chatted with mayur-sose and I know what the difference

I was doing
./vendor/bin/drush cdel canvas.js_component.NAME
So deleting the js_component config

@mayur-sose was doing
./vendor/bin/drush cdel canvas.component.js.NAME
So deleting a different thing.

I guess @mayur-sose's doing what described in the issue summary. seems like we should handle both?

For TC08 the UI does not become unusable but there error in the php log is not very helpful

AssertionError occurred during rendering of component 520bacb3-82a3-4d9e-a81a-cdc7232937c6 in Page Untitled page (-), field components: assert($js_component instanceof JavaScriptComponent)

It does not contain the ID of the component that is missing

I pushed an MR that addresses TC03 and TC08

For TC08 the error message now contains the ID of the component

Drupal\Core\Render\Component\Exception\ComponentNotFoundException occurred during rendering of component 520bacb3-82a3-4d9e-a81a-cdc7232937c6 in Page Untitled page (-), field components: The JavaScript Component with ID `noprops` does not exist.

For TC03, it makes sure the input is an empty array not NULL so there is not an exception, but we might want to change the error message in this case. See MR comment

TC03 with a component with props

I will look at the error if there is no props

I tried to test TC03

This is what I see for a page in the editor

I am able to delete the code component by using the layers menu

I also tried it with a published page

🎉 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