🇬🇧United Kingdom @thoward216

I've still not be able to reproduce TC03, following the steps and discussed with @mayur-sose, both of us on the latest 1.x with this MR. He is seeing:

What I'm seeing:

Clarified steps for TC08:

- Add an SDC component to a page
- Rename the SDC component
- Clear the cache
- Reload the page
- Try to add the SDC again to the canvas

I think that this sounds like what is described at #26 and will be handled in a follow-up #3548922: A component appearing in the list of components to instantiate that is broken during development should convey that →

I've had a look at TC03 re #42 and I can reproduce this if the code component does not have any props, if I have a component that does have props then I get the fallback as expected.

The full error I get is this:

Error 500: Drupal\canvas\Plugin\Canvas\ComponentSource\Fallback::buildComponentInstanceForm(): Argument #5 ($inputValues) must be of type array, null given, called in /var/www/html/web/modules/contrib/canvas/src/Form/ComponentInstanceForm.php on line 143

So at this point $inputs in the ComponentInstanceForm is NULL - it looks like we account for specifically if a BlockComponent source has inputs that are NULL.

      // For blocks, the client model is invalid, because $props is the "undefined" string.
      // So let's get the data from the stored tree (better than nothing)
      // @todo We require to harden this in the client-side.
      if ($inputs === NULL && $component->getComponentSource() instanceof BlockComponent) {
        $inputs = $stored_tree->getComponentTreeItemByUuid($component_instance_uuid)?->getInputs() ?? [];
      }

From my testing it looks like with any component source the inputs can be NULL at this point if there are no props defined, and I'm seeing what is mentioned in the code comment "the client model is invalid, because $props is the "undefined" string" so I'm not sure if that check should/needs to be so specific?

Taking a look at TC08.

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

Discussed this with @wimleers and @effulgentsia this looks to be an acceptable solution. MR has been created, will look at if there is a good way to test this.

Would it be acceptable to catch the EnforcedResponseException in the renderComponent function in src/Element/RenderSafeComponentContainer.php before the current catch? That appears to resolve the issue from some testing, but I maybe missing something.

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

thoward216 → created an issue.

#8 has now also need addressed in the current MR.

Moving to "needs review" as I've addressed what has been described in the ticket summary.

Comment #8 hasn't been fully addressed in this so far as it looked to be fairly straight forward but quite disruptive to the tests and add more to the current MR and wonder if its better to do this in a follow up MR or ticket?

Re #7 - The linked ticket looks to create a view page and no blocks AFAICT so should be okay.

Re #8 - It doesn't look like to be a simple case of updating those lines and has a knock on affect in lots of tests, and feels like its muddying the water a bit with this MR. When pairing with @jessebaker he suggested changing "@todo" to "Uncategorized" at least for now to be more user friendly. Happy to create a separate MR for this after this one has been merged to tackle this?

I was thinking this when looking at 📌 Disable components by default sourced from block plugins provided by core in the Standard profile Active

I think the only problem with just changing the labels is currently you can "disable" a component manually and that will then appear in the "disabled components" list with the reason of "Manually disabled", though it does also stay in the "enabled components" list. If we just change the labels it means that a "valid" component that has been manually disable would end up in the "Invalid components" list...

Maybe the solution for that is to not store a reason when a component is manually disabled anymore?

@laurii you get the following:

I've made a start on this, by default it disables all block components apart from the ones listed. A user can navigate to the components list builder to enable one if they require it.

I think a number of the tests will fail as they might be expecting more components than is now available - so these will need updating, would that be enough test coverage or do we need something more specific?

Have we tested what happens if user deletes a component and then reverts to a revision with that component?

I've just carried out the following test:

1. Create a canvas page
2. Create and add a code component to the page
3. Save the page
4. Edit the page, remove the code component and save the page
5. Delete the code component
6. Revert back to the previous revision
7. View the page in the canvas editor - the code component that has been deleted shows as 'component' in the layers menu and on the preview
8. Viewing the front end output of the page it renders without that component and no errors.

Proposed solution updated with further information.

thoward216 → created an issue.

Carrying out some manual testing, I did do some back in #19 📌 Only allow deleting Code Components if there's 0 usages Active - to confirm; previously it was possible to 'delete' a code component that was in-use if not used on the current component tree as per comment #12 📌 Only allow deleting Code Components if there's 0 usages Active .

The updates in this MR to the access check now stops a user from doing this. This MR without any FE work displays a dialog like so:

With an error in the console: "This code component is in use and cannot be deleted."

All feedback should now have been addressed.

Moving back to needs work and assigning to myself. Had a review call with @wimleers & @f.mazeikis and have a some actionable feedback.

Marking as needs review for an initial review and a few questions on the MR.

I have implemented the following endpoints as per #16:

1. /canvas/api/v0/usage/{canvas_config_entity_type_id}/{canvas_config_entity} API, which provides a simple boolean true/false response
2. /canvas/api/v0/usage/{canvas_config_entity_type_id}/{canvas_config_entity}/details API, which provides detailed of information that \Drupal\canvas\Audit\ComponentAudit can
3. /canvas/api/v0/usage/{canvas_config_entity_type_id}?page=N which allows iterating over all is paginated-per-50 and lists an object like {id1: true, id2: false, …, id50: true}

I've updated the access check in VisibleWhenDisabledCanvasConfigEntityAccessControlHandler rather than in CanvasConfigEntityAccessControlHandler which may not be correct, but we'd have to inject component audit into the other handlers that extend this which I'm not sure makes sense as its quite specific at the moment.

The above update in the access check resolves the scenario where you have a Code component added to your library that is in use on a canvas page for example and attempt to delete it, previously this would delete the code component.

I've tested #7 and this scenario looks to have been accounted for before this MR.

Thanks @wimleers, its good to question and after reviewing #16, this seems clearer to me. I've started implementing as per #16.
My only thought is that we currently only have audit data for components - but is there a view to extend that in the future and the reason that we should keep these endpoints more general?

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

Assigning back to myself to make requested changes.

I've created an MR for adding weight to Segments, I haven't addressed anything in the Drupal UI for weighting as I saw this issue: 📌 Revisit segments UI: do we need a Drupal UI? Active and I assume it may get addressed there?

I've also not addressed discussion in #2/#3 as seems thats not currently confirmed. But we might need 📌 Provide a locked Default segment Active to land first? Or it get handled in that ticket after this lands?

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

thoward216 → changed the visibility of the branch 3541501-update-phpcs-config to hidden.

thoward216 → created an issue.

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

Moving to needs review to get some eyes on the one bit of feedback that I've attempted to implement and comment added on the MR, may need further work. All other feedback should be addressed.

I believe this would be happening because when you first create a new code component it will be using the name input by the user to generate the machine name of the component. Upon renaming the component, it already has a machine name and that is something that wouldn't change once created, so it's just updating the label at that point.

I've updated some tests that were failing, updated the access check to account for the new route and added to the test.

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

After rebasing and uncommenting the schema changes as mentioned in #13, there are a number of failing tests where the validation messages are not as expected.

An example: 'versioned_properties' => 'This value should satisfy at least one of the following constraints: [1] This value should be of the correct primitive type. [2] This value should be of the correct primitive type.'

To me this doesn't look right and the message is also duplicated. I spent some time investigating a bit further with @f.mazeikis we're not sure that the constraint AtLeastOneOf can be used with sequences? Looking at the core test AtLeastOneOfConstraintValidatorTest it only ever tests with strings or integers.

thoward216 → changed the visibility of the branch 3538306-update-phpcs-config to hidden.

thoward216 → created an issue.

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

Coming back around to this, as per #10.

A new components config with one version looks like this:

uuid: d973b1d2-df01-4f89-a309-fe83f34c9d9a
langcode: en
status: true
dependencies:
  config:
    - experience_builder.js_component.test
active_version: 8fe3be948e0194e1
versioned_properties:
  active:
    settings:
      prop_field_definitions: {  }
    fallback_metadata:
      slot_definitions: {  }
    label: test
    id: js.test
    provider: null
    source: js
    source_local_id: test
    category: '@todo'

New versions created adds them under `versioned_properties` as expected but as you can see above the hash `8fe3be948e0194e1` is not in `versioned _properties`. So either the above isn't structured as expected (though I'm sure there are a number of tests around this) OR the original constraint that was commented to add later maybe stale?

Started having a brief look into this but I can't seem to reproduce. If I create two patterns with the same name the second one then gets an id as per the code in #2.

Thanks @larowlan - I've manually tested this and everything is working as expected and the output of the component tree keys is what I was expecting to see also. I've just opened an MR to run the pipeline.

I've started some investigation into this and it doesn't look to be the same issue as #3534971 and looks to be due to #3526127 and the way that the function generateComponentTreeKeys() builds the keys. The array that is passed into the generateComponentTreeKeys() function contains all 10 items in the tree in my case, but when I go through the function I only end up with 8 items.

I did a test by commenting out the set() function in Pattern entity and everything saves as expected so that looks to prove where the root cause looks to be. Will continue to debug this further.

Moving back to needs work as rebased with latest 0.x and resolved conflicts but there are now some failing tests.

Updating issue summary.

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

I think this is failing as 'versioned_properties' does not contain the hash of the 'active_version' as its simply keyed as 'active'. So I'm not sure this is the correct constraint to be added here?

Had a look into this issue and got the library attached using similar code as shown in #2 but the preview still did not appear to update as expected. Requires further investigation.

Have updated the the MR and things are looking better. Now getting errors around the validation for the version constraint that was added/uncommented.
https://git.drupalcode.org/project/experience_builder/-/jobs/5827036#L4725

XB now requires 11.2 so I believe this can be actioned.

There we also some changes in Translation test where the 'article' tag selector needed to be removed. So these may also need to be reverted/reviewed.

See: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

thoward216 → created an issue.

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

Looks like theres now a number of config schema issues now occurring: https://git.drupalcode.org/project/experience_builder/-/jobs/5657671#L2402

These looked to be "ignored" before here

Not sure if these should be resolved in this MR.

Tests are now all passing on this.

Oh yes! That was a silly mistake, have fixed that now!

I've fixed those two PHPUnit test failures, commented out the failing e2e test for now and also added a bit to the components.md - let me know if this needs expanding.