🇺🇸United States @tedbow

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

I think this is ready for review. See Issue summary for why this does not yet support dynamic prop sources and the follow-up issues where will be handled

I created 📌 Refactor(or attempt to) all or most ApiLayoutController*Test classes to also test ContentTemplate entities Active and added a todo this in the code. I did add test coverage but it is minimal right now. It covers GET, POST, PATCH and publishing of Content Templates.

I don't think we should do all the test coverage here because the Controller not in used yet for content templates and this issue blocks front end work such as ✨ Render template and support component operations in preview canvas Active . I would rather unblock the front-end and try to not have duplicated test coverage for content and content template entities in the follow-up

Added 📌 Refactor(or attempt to) all or most ApiLayoutController*Test classes to also test ContentTemplate entities Active

tedbow → created an issue.

changing because 📌 Refactor ApiLayoutController into 2 sub-class to support Content Templates Active change to not split ApiLayoutController into 2 classes. We may need do that in this issue be we shall see.

@thoward216 I think this is looking good, just need to add the test coverage I just mentioned in the MR comment. thanks!

Been working on this for a while...
Just phpunit tests we have failures...

1. CollapseComponentInputsUpdate & CollapseComponentInputsUpdateTest- fails because update .gz fixture probably needs to remade, I guess in the converted version.
2. ComponentValidationTest.php - fails at least because of component version strings
3. SegmentFormTest.php - Form field with id|name|label|value "settings[theme]" not found. // not sure why
4. SingleDirectoryComponentTest.php & CanvasTwigExtensionFiltersTest.php & ParametrizedImageStyleTest.php- probably itok replacements
5. PropShapeToFieldInstanceTest.php mega array that needs reordering

Probably a few more but I tired of looking through the logs right now. I am probably done for the day

So to be clear I didn't really have a reason to make 2 MRs versus just 1 expect when I saw the number of files changed I thought it would hard to find the script file and the changes need just for it, like composer.json scripts changes. Also wonder if gitlab UI would be laggy with so many changes.

So whatever works for people

@penyaskito another option for how to work would just be to remove of `scripts/src/ConverterCommand.php` from the 3543402-replace-experience-builder branch. The we change the script to have argument TARGET_DIRECTORY

Then to run the script you just have 2 clones of the repo locally
3543402-script-only - doesn't need to be a in drupal core directory, only used to run the script
3543402-replace-experience-builder is the actually version that will be changed

so from inside 3543402-script-only you would run composer rename -=TARGET_DIRECTORY=/path/to/other/clone

Or would it simpler to just have 1 MR? since the script doesn't change itself at all it could work.
I just made 2 MR's because the 3543402-replace-experience-builder hads thousands of file changes so I thought would be harder to look at just the script.

Not currently working on this

I ran tests/src/Kernel/ApiLayoutControllerGetTest.php locally and only 1 test failed.

The failure was because the client "type" for components has some sort of hash at the end. So it changed from

'type' => 'sdc.cv_test_sdc.two_column@d99140cbd47c0b51',
to
'type' => 'sdc.cv_test_sdc.image@cc9b97c9370aabdf',
so only the hash after `@` I always ignored this. But is this because it is somehow related the file, module, or path that has changed?

We could update all of these but also is it really more that these match exactly? Could we write `assertSameXB()` that would compare arrays and let "type" match more loosely? If so we could do this in another issue as prep

I

tedbow → created an issue.

tedbow → created an issue.

Added 📌 ContentTemplate status should be used to determine if it should be used. Active to the list

tedbow → created an issue.

I pushed a little further.

re #7 and #10. I realize now the other reason I originally refactored ApiLayoutController into a base class with 2 extending classes was that for ContentTemplates we need to also pass a "previewEntity" which would be an example entity of the bundle that would be used for the preview. The new MR doesn't cover that case. It might make sense to continue the work on the new MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1458 but then add `ApiContentTemplateController extends ApiLayoutController`. So in this case we won't need the base class of my original MR but we need a special `ApiContentTemplateController` because it needs the extra info about the preview entity and it would also need it's own routes to pass in the entity ID of the preview entity.

@thoward216 if you want to assign this back to yourself and work on it you can unassign me feel free, otherwise I will work on the preview entity problem tomorrow.

I chatted with @wim leers about #7 and why we want to create the template entity first

I pushing up some changes.

@f.mazeikis re #7 . If we create a saved, not just auto-saved, ContentTemplate entity in the UI as soon as the user wants to start working on the template won't that mean that the template will take effect immediately outside of XB? It seems that `\Drupal\experience_builder\EntityHandlers\ContentTemplateAwareViewBuilder::buildComponents` will just use the template if it exists. So as soon as we create the actual ContentTemplate config entity it would be an empty template but I think it would be used to render any entity for that bundle, meaning thy would should as blank.

Of course we could add some sort status to have the template not take effect until publishing but then we would need to set this when publishing in `\Drupal\experience_builder\Controller\ApiAutoSaveController::post` like we do for `StagedConfigUpdate` entities

if ($entity instanceof StagedConfigUpdate) {
          $entity->applyUpdateOnSave();
        }

If we are going to do this at some point we might want to create an interface for config entities that should enabled on publish or we are going keep having more entity type specific logic in ApiAutoSaveController::post

But also I am not really sure why we need to create the actual entity just because the user is given the UI for creating a new template type. The user doesn't need to know if it is just an auto-saved version at that point. Though I guess it might make the listing more complicated it could make the publishing simpler. Maybe ContentTemplates are just a unique type of config entity where we know that ID will not change and that is why they could work with just an auto-saved version before first being published🤷

Wanted to make a clarification here based on call between myself, @balintbrews, @hooroomoo and fmazeikis.

Current ContentTemplates require at least 1 dynamic prop. This makes sense but this requirement would force use to do issues in certain order and require larger issues. Since all of this work is on 1.x which does not have a release we deemed to ok if in the first few issues Content Templates don't actually require dynamic props and don't have a way to set them.

The requirement will be removed in 📌 Refactor ApiLayoutController into 2 sub-class to support Content Templates Active which is the first major back-end issue. It will be added back near the end in 📌 Require content templates to have at least one dynamic property source Active the actually requirement is 1 schema file change a little test coverage. Details in the related issues

tedbow → created an issue. See original summary → .

Will need 📌 Move `PropSourceEndpointTest` into new `XbConfigEntityHttpApiTest::testComponent()` Active

tedbow → created an issue.

This how issue is about enabling the UI. There has already been a lot of work to make the content template on the back-end. Although there are backend issues they are in support of the UI

hooroomoo → credited tedbow → .

tedbow → created an issue.

tedbow → created an issue.

Had a call with @balintbrews, @hooroomoo , @penyaskito and @f.mazeikis

We decide we would create a new meta for the front and back-end issue as this issue made before the designs and product requirements were set

Here is the new meta, 🌱 [META] Content templates Active

Changing parent to content template UI

tedbow → created an issue.

@lauriii We will need this for content templates to show the actual dynamic values. I have put a temporary fix in the spike MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

So if templates are a stable we will need this

Ok. Ready for review again

I am pretty sure at least CreateComponentTest will need to be updated for new error message format

For now to solve #26, I made sure that error in CreateComponent will still result in Yaml parsable string see https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

That is best solution I can come up with now without making larger changes

Ok. I figured out what is happening

I ran this command 2x

create a code component with 1 text prop. Make sure the machine name of the component is text1

Of course the 2nd time it will cause an error because the machine name is in use

In

$output = $sub_agent_tool->getReadableOutput();
                  $data = Yaml::parse($output);
                  foreach ($keys as $key) {
                    if (!empty($data[$key])) {
                      $response[$key] = $data[$key];
                    }
                  }

It just happens that "The component with same name already exists." will parse not throw exception as $data = Yaml::parse($output); because it happens not to have a ":" in string or any other character that could cause a problem.
so Yaml::parse($output) === "The component with same name already exists." because it might work with simple strings

the never asserts that $data is any array so this unnoticed and

if (!empty($data[$key])) {
                      $response[$key] = $data[$key];
                    }

does not pass because it is not a string. so basically the output is ignore, I think by accident, not on purpose
but the validation string for the image src problem Yaml::parse($output) will throw an exception.

so we either need a try catch around Yaml::parse($output) or better yet figure why it can't behave like SetAIGeneratedComponentStructure and gracefully handle failed attempts.

or you could just make sure $output always starts with "error:" or if it is an error and check for that

re #22 I think found what is going on. In \Drupal\xb_ai\Controller\XbBuilder::render It has special logic for these plugin classes

$map = [
        EditComponentJs::class => ['js_structure', 'props_metadata'],
        CreateComponent::class => ['component_structure'],
        CreateFieldContent:: class => ['created_content'],
        EditFieldContent:: class => ['refined_text'],
        AddMetadata::class => ['metadata'],
      ];
      $plugins = [
        'ai_agents::ai_agent::experience_builder_component_agent',
        'ai_agents::ai_agent::experience_builder_metadata_generation_agent',
        'ai_agents::ai_agent::experience_builder_title_generation_agent',
      ];

It is one of these plugins it seems to always assume getReadableOutput will be parsable yaml. So when \Drupal\xb_ai\Plugin\AiFunctionCall\CreateComponent::execute is called it first gets the validation error you mentioned "" Prop "backgroundImage" has invalid example value: [src] The property src is required" but then CreateComponent::execute is called again and now the component is created correctly and returns the yaml structure.

Problem is that in XbBuilder::render both the response from CreateComponent::execute are attempted to be parsed in
this loop

if ($sub_agent_tool instanceof $class) {
                  // @todo Refactor this after https://www.drupal.org/i/3529313 is fixed.
                  $output = $sub_agent_tool->getReadableOutput();
                  $data = Yaml::parse($output);
                  foreach ($keys as $key) {
                    if (!empty($data[$key])) {
                      $response[$key] = $data[$key];
                    }
                  }
                } 

The first one is non-yaml response which causes the error you are seeing.

I haven't had time to figure why the module needs the special logic for the these particular plugins and not for instance SetAIGeneratedComponentStructure which is able to output errors
like this
$this->setOutput(sprintf('Failed to process layout data: %s', $e->getMessage()));
and have the AI just call \Drupal\xb_ai\Plugin\AiFunctionCall\SetAIGeneratedComponentStructure::execute again to get the correct output.

So SetAIGeneratedComponentStructure does not have problem that failed attempted results in a plain string and successful attempt results in json string. But CreateComponent is set up not to work that way.

My suspicion is that if CreateComponent hit the existing error

$this->setOutput("The component with same name already exists.");

It would also fail to work correclty because it would try treat this as yaml in \Drupal\xb_ai\Controller\XbBuilder::render

There is test coverage in \Drupal\Tests\xb_ai\Kernel\Plugin\AiFunctionCall\CreateComponentTest::testCreateExistingComponentFails but this does not test the output with \Drupal\xb_ai\Controller\XbBuilder::render

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

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

tedbow → created an issue.

The phpstan errors are from 📌 Fix phpstan in HEAD from phpstan update to 2.1.22 Active so won't stop this from being merged

@mglaman, can you take a look?