🇦🇺Australia @larowlan

The backtrace indicates this is coming from the download count module

Thanks for a detailed bug report 👌

This is ready for review, there' s a failing cypress test that is failing in HEAD

Doesn't look like that worked

Also tried moving that test to playwright but for some reason the all-props component breaks the preview in playwright

I think we probably need to revert this until we can work out why it makes the cypress test fail

Added some waiting to the prop-types test that has been failing

I think this is ready now

In the artifacts for the failing prop-types format=iri-reference test, I can see it is still waiting for the PATCH when it fails.
I'll add a wait and see how it goes. Would prefer to move forward than revert if we can.

I think your proposed pseudo approach is worth a shot - nice one

#3557751: Add tests for Multiple image props on SDC are not supported in Canvas → here's the follow up

larowlan → created an issue.

https://www.drupal.org/project/link_attributes/releases/2.1.2 →

Cutting 2.1.2
Thanks

Good pick up. I wonder how you have no sessions table if you're logged in - redis?
I guess we need to find a concrete table in a hook_schema and use that instead

Just a minor suggestion on the MR, thanks for working on this!

+++ b/microcontent.module
@@ -62,3 +63,10 @@ function microcontent_jsonapi_microcontent_filter_access(EntityTypeInterface $en
\ No newline at end of file

minor nit, missing a new line here

This feels reasonable. There are still a lot of WBM sites for better or worse.

Any chance you could convert this to an MR so we can have a test run?

Thanks for working on it

Took me 2 and a bit weeks, but finally got around to reviewing.
Thanks for working on this. Left some comments on the MR. This approach looks really nice!

https://www.drupal.org/project/dbal/releases/2.0.3 →

Fixed, thanks, cutting a new release tag

Great idea. Couple of minor nits on the MR
It would be good to ensure we have a test that `::getTree` returns a fresh tree after adding a new term.
The test could be as simple as calling `::getTree`, assert the contents, add a term to the vocab, call it again, assert the new term.

Thanks!

Great find. Any chance we could add a test?

My guess is this was either by accident or for auditing sake.

I would be open to making it configurable

🏗️

Left a comment about a possible performance improvement

I think perhaps this could/should be a child of 🌱 Plan to allow editing props and slots for exposed code components Active as it is dealing with the practical implications.

e.g with 📌 Support deleting a prop from an in-use code component Active and 📌 Test coverage to prove `GeneratedFieldExplicitInputUxComponentSourceBase::validateComponentInput()` disallows garbage values Active we can't actually do GC because if the prop exists in the older version and that version is in use, we have to retain the values. But upgrading to the latest version allows us to do that GC

Thoughts?

poker10 → credited larowlan → .

poker10 → credited larowlan → .

mcdruid → credited larowlan → .

greggles → credited larowlan → .

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

Looking at \Drupal\Tests\canvas\Functional\CanvasConfigEntityHttpApiTest::testJavaScriptComponent

I can see we have a test to prevent deletion:

    // We can NOT delete the 'test' Code Component via the Canvas HTTP API because it is in use.
    self::assertTrue(\Drupal::service(ComponentAudit::class)->hasUsages($component));
    $body = $this->assertExpectedResponse('DELETE', Url::fromUri('base:/canvas/api/v0/config/js_component/test'), [], 403, NULL, NULL, NULL, NULL);

But I can't see anything that checks we can't change the status if the code component is in use.

larowlan → created an issue.

Assigning to Wim for a sanity check of the current progress/approach before we go any further.

There is certainly more refactoring we can do here - such as

• #3526045-7: Consider renaming ComponentSource plugins to ComponentType → but I think the current state fixes the stable blocker and we can work on further refactoring as the source plugin API solidifies.
• Saving less config entities when we get an invalidation by querying by the unique url schema for the prop shape that changed

I'm done for today, but I've moved the needle on MR 145. I've left @todos for breadcrumbs where things need work

Working on this in MR 145

Still getting the assert error in manual testing that this hunk was to prevent

😍 This looks amazing

Hey folks, first step here is an explain of the SQL per #2

Very happy to add indexes etc that help - but we need to diagnose them first

Got a start on this, its at least working

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

Added the missing FE bits

Our tests were testing the Drupal form but not the hyperscriptify bits that turn that into React things.

Added cypress tests for that

Here's a screeny so removing that tag

The link widget help is a lot. We might want to explore making it something else.

Added some tests that demonstrate the 500 errors I was getting in manually testing yesterday

They fail without the latest changes

This is ready for review again

Discussed this with @alexpott and @catch

The primary reason this was tagged as 11.3.0 release priority was to enable ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed which is only interested in json.

If we defer an allow list of extensions to a follow-up we would need to get both issues into 11.3 as restricting after the fact would be an API break.

So instead we feel it is best to limit it via a container param and default to json only in core.

Setting to needs work for that.

I would suggest handling the file extension limitation to a follow-up,

I think this is reasonable.

The system.files download would only be possible if someone had an overly permissive hook_file_download (there are no such implementations in core). So the regex limitation there was just to be super cautious.

I think 446 comments is enough

Thanks for accommodation the route requirement change @alexpott - and yeah, that route is very odd

I'm being super paranoid here, but I think we should add a requirement to the system.files route (via a regex) that prohibits module and theme as valid schemes for the /system/files/{scheme} path

Yes, we'd need a hook_file_download to return a header - but I'd hate for a dud hook_file_download in contrib to lead to a vector where people can download the source code of modules/themes - yes many themes/modules come straight from Drupal.org but most projects have custom code.

I think its a simple addition to give us some additional hardening.

larowlan → changed the visibility of the branch 3470422-be-nice-for-sdc-developers-larowlan to hidden.

larowlan → changed the visibility of the branch 3470422-be-nice-for-sdc-developers to hidden.

Added child issues and updated the issue summary to reference them

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

I postponed #3556327: [PP-1] Don't save component config entities during discovery in an update kernel → on this because the new tests here will give us coverage for it.

Postponing on 🐛 Text formatted with CKEditor within Canvas gets double escaped when output Needs work because we'll get test coverage for free from that, the number of versions for the SDC component we update will now be 3 instead of 2.

Opened #3556327: Don't save component config entities during discovery in an update kernel → for #18

A comment from @mglaman on 🐛 Canvas' hook_storage_prop_shape_alter() must allow passing cache tags — to know when to re-evaluate Active made me realise the 2 vs 3 versions in the update test is actually a symptom of a bigger issue - so I opened #3556327: Don't save component config entities during discovery in an update kernel →

larowlan → created an issue.

😱 I didn't even think about the fact UpdateKernel uses NullBackend so the plugin managers are always calling setCachedDefinitions and causing component churn, unless the update hooks mark config as syncing.

We should consider opening a new issue to prevent this, e.g. by checking if we're in an update kernel in the component manager and block manager before we blindly save new entities - if this happens during database updates we're breaking the rules around what you can do in an update kernel vs what should wait for the post update hooks. Namely, using the Entity APIs is not supported in an update kernel until post update hooks.

Turns out hook_media_type_insert is too early, because at that point the source field doesn't yet exist. So used hook_field_config_insert and the equivalent update hook instead. Works just fine!

This doesn't cover any contrib-based source plugin that extends from \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase

But I think we can live with that.

larowlan → changed the visibility of the branch 3547579-hook_storage_prop_shape_alter-allow-specifying-when-to-reevaluate to hidden.

Working MR up https://git.drupalcode.org/project/canvas/-/merge_requests/298

larowlan → created an issue.

Yes we had the same issue with automatic regeneration in 🐛 Text formatted with CKEditor within Canvas gets double escaped when output Needs work yesterday - i.e. the fact SDC component config entities can be resaved on a cold cache during discovery meant that update hook changes were applied during discovery rather than during the post update hook.

The same applies for code components.

I think yeah we need a hook_media_type_insert/hook_media_type_update here to flag that we need to regenerate code component config entities - we can query for JavascriptComponents with a prop of type `$ref: 'json-schema-definitions://canvas.module/image'` and resave them

I'll have a crack at that

Thanks

Is there any way we can write test coverage for this?

We have existing examples in core that create modules on the fly during the test - see \Drupal\Tests\system\Functional\Theme\MaintenanceThemeUpdateRegistryTest::prepareEnvironment we could write a test module to $this->siteDirectory . '/modules/' that has a block plugin, and then delete it and assert the expected behaviour

ah updateExistingComponentValues is used in a test component ConceptProver - is that module/test coverage still needed? the module name is canvas_test_extension_legacy

I think we can remove updateExistingComponentValues if it's not used, no sense in having API in a stable and then needing to support it forever.

For _updateExistingComponentValuesForLinking - I agree - defer that to when we have more tests

I'll update the MR to drop updateExistingComponentValues

Attempting to update the issue summary to reflect the comments that track how this progressed from 3 items to just 1 and the current state of the MR.

Code changes look good to me - I think this is RTBC

Current code looks good to me and has product manager signoff in #6

Re #19 I think there is merit in a UI schema but that is out of scope for here and would need to be something we did in core as a feature request - can you open an issue for that?

Left two comments that I think can likely be followups given this is RTBC and critical

Couple of test fails here that look related to assumptions about the previous behaviour

This is a great improvement 👏

This caused a fatal error on 11.x for Canvas which has a semi_coupled theme engine.

Is there a BC layer that should work until any existing engine has a chance to update?

Error 500: Service "semi_coupled" not found: the container inside "Drupal\Core\Theme\ThemeManager" is a smaller service locator that only knows about the "twig" service.

Left a review on MR 196 (Which I think is the right branch to review based on recent comments)

Marking as needs work for XSS safety.

Left some questions, but this looks nice

We need a change record here and we aren't actually emitting a deprecation error yet for those with components in their default config.

Nice one!

I confirmed all the edited css files are just generated with this script

git diff origin/11.x --name-only|grep css|while read r;do J=${r/\.css/.pcss.css};if [ ! -f "$J" ]; then echo $J;fi;done

Which printed nothing 👌

Committed to 11.x, backported to 11.3

Updated the solution to make use of the ajaxStop event Drupal fires and was able to remove the need for timeouts
Added a playwright test that replicates @bnjmnm's excellent debugging and changes to the test module

If you want to manually test this, revert commit 8cc00e69023d8b49f5b8efeb1c46a74dde914539

poker10 → credited larowlan → .

This was resolved in 📌 [PP-2] Accept Dynamic Props in ApiTemplateLayoutController for content templates Postponed which removed that function in favour of \Drupal\canvas\Controller\ApiLayoutController::getEntityWithComponentInstance which uses \Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemList::getComponentTreeItemByUuid

📌 Replace the postPreview action with atomic equivalents Active most likely won't go in before stable

larowlan → changed the visibility of the branch 3539693-inputbehaviourscommon-mutates-the to hidden.

Rebased as an MR against Canvas, refined reasoning and title

larowlan → changed the visibility of the branch 3539693-inputbehaviourscommon-overwrites-the to hidden.