larowlan → made their first commit to this issue’s fork.
Updated the MR based on my review - but I'm wondering if we should just do GC on those properties in \Drupal\canvas\ComponentSource\ComponentSourceInterface::clientModelToInput instead rather than raise an Exception?
larowlan → made their first commit to this issue’s fork.
larowlan → made their first commit to this issue’s fork.
larowlan → made their first commit to this issue’s fork.
Confirmed that's what we're doing in the current branch - the children are lost when we do server side conversion upon post.
@lauriii can you clarify how this should work with respect to children inside the slot that is removed.
Should they become a sibling of the parent from where the slot was removed or should they also be removed?
The backend already supports this, and the FE changes are happening in 📌 Frontend changes to allow editing props and slots for code components Active
This will most likely be closed as duplicate when 📌 Frontend changes to allow editing props and slots for code components Active is complete
📌 Support adding a new required prop with an example to an in-use code component Active solves this too
We've split off 📌 Frontend changes to allow editing props and slots for code components Active for the FE changes and this will just be about BE changes.
Picking it up
The backend already supports this, and the FE changes are happening in 📌 Frontend changes to allow editing props and slots for code components Active
This will most likely be closed as duplicate when 📌 Frontend changes to allow editing props and slots for code components Active is complete
Refactored this to only be about the backend changes
I think we probably want to tackle two things here:
* the resolvers probably need to be able to say whether they replace each other - something like 'stopEventPropogation' does for events to say, don't run stuff after me - so in that scenario the entity usage one would say - don't run any other resolvers. It might be something like `isAuthorative` method on the interface
* adding the new method to filter out the self references is a good idea - but I wonder if we could just modify the interface from `getFileUsages($file)` to `getFileUsages($file, MediaInterface $self)` and remove the bit in the existing code that does the `$usages - 1`
Either is a BC break - either we're adding a new method or we're changing the signature. I think if we did `getFileUsages($file, ?MediaInterface $media = NULL)` we could trigger a deprecation warning if $media was NULL and do the whole 'this will be required in 2.x version' BC layer. Then we could release a new 2.x version with $media required.
But yeah either way we're looking at a new minor version because deprecations and a new major with the BC breaks.
But happy to do all that release management work because it improves the module's usefulness
Discussed with @jptaranto and from a front end point of view it doesn't make much sense to do this separately to 📌 Support adding a new optional prop to an in-use code component Active
Postponing for now and the code for this feature will be in that story.
Committed - thanks for adding tests, will cut a new release with this in it
Done! https://www.drupal.org/project/field_group_dl/releases/1.0.3 → thanks for the prod
Thanks, technically the drupal.org packagist facade derives these from the info.yml files and they're redundant if there' s no external (non drupal module) dependencies, but no harm in having it
Thanks, this seems like a no-brainer - can we add additional asserts/logic to the existing tests to add coverage for this functionality?
Thanks!
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
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
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!
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?
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.
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