Canvas AI: CanvasAiComponentDescriptionSettingsForm redirects to 'canvas.api.config.list' after submission

buildForm() and SubmitForm() methods in the form calls Drupal\canvas_ai\CanvasAiPageBuilderHelper::getAllComponentsKeyedBySource to get the active components. Under the hood getAllComponentsKeyedBySource triggers a sub request to the 'canvas.api.config.list' route get fetch available components from the API. For some reason, the Form api considers this request as the current request and redirects users once the form is sumitted

Thanks for reviewing this @narendrar. While reworking this, I feel that the sub-requests added in #3545477: Use subrequests when getting component list in `::getAllComponentsKeyedBySource()` might not be needed. This sub-request gets fired every time the form is loaded and/or submitted (and also when get_component_context gets invoked (Drupal\canvas_ai\CanvasAiPageBuilderHelper::getComponentContextForAi)). Even if we cache the results, the sub-request will get fired at least once when the user loads the form for the first time. Due to this, the form title won't get displayed as it interferes with the form request.

I feel the better approach is to bring back the original logic of invoking the controller method here, do the necessary calculations, and then cache the result. This eliminates the sub-request and thus fixes the issue with the form.

Regarding the tests, I think it makes more sense to add a test for the get_component_context tool to ensure that
1. If user doesnt use the form, this tool returns the components with their default descriptions
2. If user uses the form to override component descriptions, the tool returns those.

That can be it's own separate issue. What do you think?

I have added the following tests
1. CanvasAiComponentDescriptionSettingsFormTest.php – verifies that the form title is rendered correctly and that form validation and submission behave as expected.
2. CanvasAiPageBuilderHelperTest.php – checks whether the component list is properly cached and ensures that the cache is invalidated when components are updated.

While creating the CanvasAiComponentDescriptionSettingsFormTest, I encountered the following error when submitting the form in the test:

Schema errors for canvas_ai.component_description.settings with the following errors: canvas_ai.component_description.settings:component_context.block missing schema in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 98 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php).

This error does not occur when manually saving the form from the UI.
To work around this in the test, I had to add: protected $strictConfigSchema = FALSE; in the test. Will create a followup issue to check this further.

We also need additional tests to verify:

• When a component/prop/slot description is overridden using the form, the tool should return the overridden description.
• When components from a particular source (e.g. Blocks) are disabled using the form, the get_component_context tool should return only the components from the enabled sources.

These tests can be incorporated along with #3545816: Simple approach to bringing advanced metadata into Canvas AI, as that issue modifies the output of the get_component_context tool.

Edit: Created #3560231: Canvas AI: Define proper config schema for CanvasAiComponentDescriptionSettingsForm

Updating the tests

Ready again

The schema related error mentioned in #13 have been resolved now.

Thanks, Scott. Some changes I had earlier in MR 331 are no longer part of the MR now. I am not sure how they got reverted.

The missing changes are:
1. A functional test for the form: https://git.drupalcode.org/project/canvas/-/blob/2d7eda23c9f693af965499e...
2. Some additional changes needed in CanvasAiPageBuilderHelper::processSdc to fix certain bugs identified while running the above test

Since we now have two separate MRs, I am not sure which one these changes should be added to.

Thanks, Scott. That sounds good to me.

The functional test I added already verifies multiple aspects of the form, including:

• The form title is displayed correctly
• Validations are triggered as expected
• Default values appear correctly after form submission
• The user remains on the form after submit

So i think the test you added (CanvasAiFormRedirectTest) may not be required.

I had also added a kernel test to verify that component data is properly cached:
canvas_ai/tests/src/Kernel/CanvasAiPageBuilderHelperTest.php::testGetAllComponentsKeyedBySourceCaching()
Looks like something went wrong during a rebase on my side, so feel free to go ahead and push the changes directly to my MR. If anything here is unclear or confusing, please let me know. I am also happy to handle it from my side if that is easier.

TL;DR – changes to be included:

• Functional test for the form: https://git.drupalcode.org/project/canvas/-/blob/2d7eda23c9f693af965499e...
• Kernel test for caching: https://git.drupalcode.org/project/canvas/-/blob/2d7eda23c9f693af965499e...
• processSdc function changes: https://git.drupalcode.org/project/canvas/-/blob/2d7eda23c9f693af965499e...

I incorporated the config schema and caching-related changes from MR !524, since those caching updates were more robust than my original approach.
While testing everything locally, I found an issue related to caching, which I've commented on in MR !524, along with the fix.

MR !331 now contains all the required changes. Could you please review it so we can close !524?

Ahh… previously, the test CanvasAiPageBuilderHelperTest::testGetAllComponentsKeyedBySourceCaching used a static cache ID to verify that the cache was invalidated correctly when a component was disabled. However, we no longer have a static cache ID, so the test is now failing.

Can we make the methods getComponentListCacheability() and buildCacheId() public?

Also, since we are already caching the results, do we still need the in memory caching via CanvasAiPageBuilderHelper::$fetchedComponents?

Thanks for updating the test. I think we should consider removing it, as it exposes an edge case where the cache is invalidated but a stale component context is still shared with the user.

I was able to confirm this behavior with the following steps.

In CanvasAiPageBuilderHelper::getAllComponentsKeyedBySource, I added a log statement'echo "Using memory cached components" inside

    if ($this->fetchedComponents !== NULL) {
      echo "Using memory cached components\n";
      return $this->fetchedComponents;
    }

Then Called $this->canvasAiPageBuilderHelper->getAllComponentsKeyedBySource(); again in CanvasAiPageBuilderHelperTest::testGetAllComponentsKeyedBySourceCaching() just above $this->assertFalse($cached2, 'Cache is invalidated after component disable');

Since the test disables a component, calling getAllComponentsKeyedBySource() again should fetch the components from the source. However, I still saw the log message "Using memory cached components". This indicates that even though the cache was invalidated, the in-memory cache remained in effect, resulting in stale data being reused.

Ran the test again in local and it works as expected. Thanks.
BTW, I am not sure if I am the right person to move this to RTBC, since I have also partially worked on it 😅.

#3573571: Use VariationCache for getAllComponentsKeyedBySource() cache context handling would fix this issue in a much better way. Sorry, I added it as a comment in that issue not this one.
@jibran, it would be helpful if you could review #3573571: Use VariationCache for getAllComponentsKeyedBySource() cache context handling instead.