Save Multi-Value Prop Configuration for Code Components

AFAICT this is the blocking back-end piece? If so: updating issue metadata.

FYI see the video at #3571917-12: UI Support for Multi-Value Prop Configuration for Code Components that demonstrates the UI.

This bit in it has me wondering:

… how do we model that in SDC metadata and in equivalent code component config entity representation? In SDC we definitely can: tests/modules/canvas_test_sdc/components/sparkline/sparkline.component.yml does this. But we want to be sure about code components, too.

→ that would need explicit \Drupal\Tests\canvas\Kernel\Config\JavaScriptComponentValidationTest test coverage additions.

The multi-value property support is now fully functional. My next steps are to refactor the code, implement unit tests, and resolve the current CI pipeline failures.

Build is green! 🟢 I'm going to wrap this up by adding test coverage for the new code and doing some final deep testing

Added required changes to support Save Multi-Value Prop Configuration for Code Components, hence NR.

Rebase from UI branch causes some issue, I have fixed all of it, now its ready for review.

Found one issue while testing:

• Create an image field, mark it as required.
• Check 'Allow multiple values'
• Click 'Add to components', and Add
• Component breaks rendering

Similar is the case with Integer field, if we add decimal values and try to Add component, we can not recover from this state.

Required Image field related issue reported in #10 also exists in 1.x. Hence create new issue #3575410: Code component editor does not require providing an example for required "image" props, then results in 500

@chandu7929 please clarify the relationship between this MR and the one in #3571917: UI Support for Multi-Value Prop Configuration for Code Components

I updated the MR description based on your update to the issue summary. You added

MR of this issue contains changes from UI issue: https://www.drupal.org/project/canvas/issues/3571917 please keep this in mind while reviewing the changes.

Which makes me think the current MR will not commit the changes to /ui and they are just there for manual testing, but then in #7 you said

Build is green! 🟢 I'm going to wrap this up by adding test coverage for the new code and doing some final deep testing

Then you added 1 commit with test coverage https://git.drupalcode.org/project/canvas/-/merge_requests/567/diffs?com...
But that test coverage is under /ui, so without the other UI changes, these won't pass. So that makes me think we will be committing *all* the changes under /ui, which I am guessing is not correct, hence the need for clarification in the summary.

We really need a phpunit test for this. See @wimleers' comment in #5. Basically, for every change under /src, we need to be asking, "does this need phpunit test changes? Where?"

When you set it to "needs review" in #8, as far as I can tell, there was only a 1-line change under /tests/src https://git.drupalcode.org/project/canvas/-/merge_requests/567/diffs#ddc...

I added some test coverage but only in response to @NarendraR's comment https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_704589 and while trying to unblock that specific point. As a reviewer, I am not going to add all the test coverage needed, and it should be added before setting to Needs Review (or adding a comment as to why it has not been yet).

see #14

@chandu7929 re the test coverage @wimleers asked for in #5

→ that would need explicit \Drupal\Tests\canvas\Kernel\Config\JavaScriptComponentValidationTest test coverage additions.

That is now in https://git.drupalcode.org/project/canvas/-/merge_requests/645

I think it makes more sense to get that issue committed first @isholgueras I think is going to take that back over today

@tedbow - With respect to comment #14:

@chandu7929 please clarify the relationship between this MR and the one in #3571917: UI Support for Multi-Value Prop Configuration for Code Components

I updated the MR description based on your update to the issue summary. You added

MR of this issue contains changes from UI issue: https://www.drupal.org/project/canvas/issues/3571917 please keep this in mind while reviewing the changes.

I already mention over our standup call, we are including UI changes to verify overall functionality of this ticket without it we won't be able to test the functionality.

Which makes me think the current MR will not commit the changes to /ui and they are just there for manual testing, but then in #7 you said

Build is green! 🟢 I'm going to wrap this up by adding test coverage for the new code and doing some final deep testing

Then you added 1 commit with test coverage https://git.drupalcode.org/project/canvas/-/merge_requests/567/diffs?com...
But that test coverage is under /ui, so without the other UI changes, these won't pass. So that makes me think we will be committing *all* the changes under /ui, which I am guessing is not correct, hence the need for clarification in the summary.

Again, As I mention UI changes will require to keep this functionality working it means UI changes will go first then this ticket changes will get merge. In the test I have covered all multi-value support which is needed for this ticket.

We really need a phpunit test for this. See @wimleers' comment in #5. Basically, for every change under /src, we need to be asking, "does this need phpunit test changes? Where?"

When you set it to "needs review" in #8, as far as I can tell, there was only a 1-line change under /tests/src https://git.drupalcode.org/project/canvas/-/merge_requests/567/diffs#ddc...

I don't see the reason not to move in review if all test are coverd for the code I have written.

I added some test coverage but only in response to @NarendraR's comment https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_704589 and while trying to unblock that specific point. As a reviewer, I am not going to add all the test coverage needed, and it should be added before setting to Needs Review (or adding a comment as to why it has not been yet).

I would request reviewer to just review and provide feedback instead on working on the issue directly, let developer work on the feedback otherwise it will be hard for developer to follow what is completed and what's pending.

@tedbow - After looking at #16 and #17, I don't think anything is pending now, please let me know if that's not the case, except for fixing the CI.

Rebased from UI branch.

Started to review but I am not clear on what the difference is between the 2 MRs. One branch name ends in "on-1.x" but they are both against 1.x🤔 and new MR has no description and there was no comments to why it was opened.

Anyways, I was reviewing !567 but for some reason it is still showing differences with 1.x that don't make sense, see https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_707104 and https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_707113

So I am going to hold off on reviewing until it is clear which MR I should be reviewing.

@chandu7929 let me know which MR should be reviewed. It looks like they both have /ui changes from #3571917: UI Support for Multi-Value Prop Configuration for Code Components.

I am guessing in #20

Rebased from UI branch.

you meant you were creating a new MR that had the latest changes from #3571917: UI Support for Multi-Value Prop Configuration for Code Components. It sounded like you were rebasing the original MR, also because it was posted before #21, the system message with "chandu7929 opened merge request !649". Maybe a drupal.org glitch 🤷

Anyways, if just 1 MR should be reviewed, please close the other MR for clarity. If they both should be open, please explain the purpose of each.

@chandu7929 I remember in our call you did mention opening up another MR but looked at many other things after the call before I got back to this. Please document the reasons for those actions on the issue, for me or anyone else who might be looking at the issue and wasn't on the call. It doesn't need to be long comment. It could just be

Opening a new MR, please it and ignore !567

But ideally you would close the old one too if it really is outdated

I marked the branch 572553-multivalue-props-on-1x as hidden, as it not require anymore.

Latest rebase from UI branch.

Combine all changes for this issue in single commit for ease of review.

@tedbow - Combined all changes for this issue in single commit for ease of review, please have a look.

See MR comments.

I documented which files will be committed in #3568264: Update code component props schema to support 'array' type first but there are still some threads to be addressed in this issue

Addressed all feedback and suggestions from MR; moving back to review.

Rebase with latest 1.x, since UI branch is merged.

re https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_709870

I think after #3571917: UI Support for Multi-Value Prop Configuration for Code Components we shouldn't see so many /ui changes

From summary

This ticket is limited to saving and retrieving the configuration only. All UI behavior and interactions are handled in a separate ticket

so still needs "Needs issue summary update"

So setting to "Needs work" but to be clear I can still review the other /src portions, I am not blocked on that

I think after #3571917: UI Support for Multi-Value Prop Configuration for Code Components we shouldn't see so many /ui changes

From summary

This ticket is limited to saving and retrieving the configuration only. All UI behavior and interactions are handled in a separate ticket

so still needs "Needs issue summary update"

So setting to "Needs work" but to be clear I can still review the other /src portions, I am not blocked on that

@tebow - These UI changes are specific to this ticket. They address a regression from version 1.x where component form values failed to persist/display after saving.

As per summary: Persist the multi-value prop configuration defined in the Component Builder UI so that component props can be correctly retrieved and reloaded in the Canvas editor.

@chandu7929 thanks for addressing the latest point. As per our chat I am going to work on this issue for the next couple days until you are back on it

@chandu7929 once you are working on this again please stop git force pushing and wiping out the commit history. It makes it very hard to review and work on this. Right now the MR only has 3 commits on it. If the commit history existed after MR comments I can often find the git commit the relates to the MR comment but it is not possible now

Ping me if you are having problems bringing in the latest 1.x changes without forcing pusing

updating summary now that #3571917: UI Support for Multi-Value Prop Configuration for Code Components is merged

@chandu7929 I leaving it tagged as "Needs issue summary update" because there is still info I need to you to update regarding you comment in #32. I left placeholder in the summary

After I merged in 1.x about 17 phpunit started to fail https://git.drupalcode.org/project/canvas/-/pipelines/758486/test_report...

not sure why yet

It might be related to schema related changes I brought in from #3568264: Update code component props schema to support 'array' type. That issue was green but I merged it with 1.x also to see if there are the same test failures there also . running https://git.drupalcode.org/project/canvas/-/pipelines/758601

UPDATE: that job ran with just 1 test fail for new test there, so still have to figure out what is causing the 13 phpunit test fails in this MR

Regarding #36, I think this issue should be postponed on #3568264: Update code component props schema to support 'array' type. I tested locally and if I revert bringing in those changes it does cause of some of the schema validation errors in test to go away. The problem I think the schema changes in #3568264: Update code component props schema to support 'array' type are probably more correct. Hopefully that issue will be. committed soon(tomorrow?)

I think trying to get the tests passing here is waste of time right now because if there are further changes to #3568264: Update code component props schema to support 'array' type they will break again when it is commited.

@chandu7929 I have some open questions for you on the MR and those would still be good to get answers on while we are waiting on #3568264, just further code changes should probably wait

I think this issue should be postponed on

💯 — I don't understand why it hadn't been already. The issue summary is in dire need of an update.

I tagged this Configuration schema in #3, but that's because this only linked to #3571917: UI Support for Multi-Value Prop Configuration for Code Components, even though #3568264: Update code component props schema to support 'array' type predates this. The original issue summary is what AFAICT caused this confusing situation.

In fact, the issue summary is so confusing that I'm marking this Needs work instead. While this waits for that issue to land, we should make it clear what the scope/intent is here, and how it relates to/builds upon other issues. 🙏

FYI once this lands, I predict @lauriii will identify #3522718: [later phase] [needs design] UX for associating mismatched cardinality field instance (too little or too much) with a higher cardinality SDC prop (e.g. `type: array, maxItems: 5`) or lower cardinality (e.g. `type: string`) (or create a duplicate) as an important next issue 🤓

@tedbow - With respect to your comment #37, I verify the changes applying from SCP-261especially this commit and removing the same file changes from branch 3572553-save-multi-value-prop, the following issues occur:

1. Image with limited/unlimited cardinality

2 Errors
Prop "imageLimited" has invalid example value: [[0]] Array value found, but an object is required [[1]] Array value found, but an object is required

2. Video with limited/unlimited cardinality

Prop "videoLimited" has invalid example value: [[0]] Array value found, but an object is required [[1]] Array value found, but an object is required

3. List text limited/unlimited cardinality

2 Errors
'enum' is not a supported key.
'meta:enum' is not a supported key.

4. List number with limited/unlimited cardinality.

2 Errors
'enum' is not a supported key.
'meta:enum' is not a supported key.

So, there's no point in blocking this ticket. We must keep our version of the following files to resolve the issues and fix the failing test.

• config/schema/canvas.json_schema.yml
• modules/canvas_dev_mode/src/Hook/UsePrivateApis.php
• src/Config/Schema/JsonSchemaObject.php
• tests/src/Kernel/Config/JavascriptComponentStorageTest.php

#3568264: Update code component props schema to support 'array' type is now in!

So, there's no point in blocking this ticket. We must keep our version of the following files to resolve the issues and fix the failing test.

@chandu7929 thanks for investigating. I think, though, that the failures might be legit failures now that the schema has changed. But you mentioned in chat that we need to keep our changes on top of the ones from #3568264: Update code component props schema to support 'array' type. You may be correct.

For now, though, I am going to bring the changes in from 1.x (made in #3568264) exactly as they are so we can get a GitLab CI run with the failures.

re
modules/canvas_dev_mode/src/Hook/UsePrivateApis.php
I don't think we need this anymore. I probably am the one who originally suggested making the schema changes dependent on canvas_dev_mode, but I think that is overly cautious. #3571917: UI Support for Multi-Value Prop Configuration for Code Components already will not show the new UI if canvas_dev_mode is not enabled, so I do not think we need to have the schema be dependent on this.

re src/Config/Schema/JsonSchemaObject.php b/src/Config/Schema/JsonSchemaObject.php. I checked and the only difference is code comments.

re ests/src/Kernel/Config/JavascriptComponentStorageTest.php there is a small test expectation.

I am going to merge in 1.x

Looking at failures

1. Tagging needs follow-up because I removing unrelated changes see https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_710320

   commit https://git.drupalcode.org/project/canvas/-/merge_requests/567/diffs?com...

2. Remove not in summary about duplicate files from #3568264: Update code component props schema to support 'array' type, as this was committed
3. I agree with @wim-leers regarding in #38 and #39 the summary confusing enough to where it needs to be fixed before it can be worked on.

   but I think part of the problem is the current MR tries to do much. We need to fix the scope. See my next point

4. This will has parts, validate schema of JavaScriptComponent, does normalization for the client, and fixes UI parts from #3571917: UI Support for Multi-Value Prop Configuration for Code Components.
   We postponed it on #3568264: Update code component props schema to support 'array' type but this issue I think has at least 1 change that probably should have been done there, the changes to src/ComponentMetadataRequirementsChecker.php and the related test coverage changes, which since they bring in a new test component affect a few files.

   I am going to up an MR here, that could mergeed by itself. It might better to move that to its own issue, or just a quick follow-up on #3568264 itself but to save time I am going to add it here.

re #45 4) I have created a new merge https://git.drupalcode.org/project/canvas/-/merge_requests/706

which updates \Drupal\canvas\ComponentMetadataRequirementsChecker::check to valid enum and meta:enum

If we can merge this first it will help get https://git.drupalcode.org/project/canvas/-/merge_requests/567 into something that is more managable.

@wim-leers if you want we can split !706 into its own issue just re-open #3568264: Update code component props schema to support 'array' type instead, and add this a follow-up MR there. It feels like left over from that issue.

@tedbow - I don't see the point of creating a separate MR!706 and blocking other merge requests MR!567 for a single line of change you added here . Apart from some tests which are already there in the main branch, we could have worked directly on the main MR and saved two days.

I will validate the main MR!567 with the latest changes and proceed as needed. Let's avoid such practices to prevent wasted time and delayed tickets.

Please let me know if I misunderstood.

@chandu7929 re #49

@tedbow - I don't see the point of creating a separate MR!706 and blocking other merge requests MR!567 for a single line of change you added here .

MR !567 had, and probably still has unyielding scope that, at least for me, makes it hard to review. It is just tackling too many things: schema, validation changes, backend client request normalization, and UI fixes. It has been made worse because the issue summary was very out of date at the time I made !706.

!706 extracted schema and validation changes for one file and the related test changes. There are 8 files total, and it was very clear how the production changes were related to the changes.

It allowed improvements to the one non-test file, not just a one-line change. The one-line change just got rid of the last PHP warning in the test.
I should have committed the file exactly as it was in the other branch first. Sorry, I meant to. If you want to see the first difference, run git diff 4b16d8870fdc50c4fcfa11459c55e4829ef11740 -- src/ComponentMetadataRequirementsChecker.php

Let's avoid such practices to prevent wasted time and delayed tickets.

Sorry, I respectfully disagree that this has wasted time on the issue. !706 made a well-defined MR that could be committed quickly, leaving !567 closer to manageable and reviewable scope.

Reviewing

MR567 is ready for review from UI perspective as well. No further changes. some tests are failing which need to be fixed.

Added some feedback!

Setting to NR since I have addressed all feedback related to FE

Just one more nit!!

@chandu7929 can look into making these 2 follow-ups if these are still issues

https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_722228

https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_722237

Thanks

Progress++ — so +1 to merging this to unblock more progress 😊

@tedbow walked me through the parts he was most concerned about. The one that stood out the most to me: the changes to \Drupal\canvas\Entity\JavaScriptComponent::updateFromClientSide() to auto-fix invalid props provided by the client must be reverted. Specifically, the entirety of \Drupal\canvas\Entity\JavaScriptComponent::normalizePropsSchema() must be reverted — that is pure technical debt being added, and is literally opposed to what #3568264: Update code component props schema to support 'array' type did, which is what this was blocked on in the first place! Details on the MR.

@wimleers assign to your for review on https://git.drupalcode.org/project/canvas/-/merge_requests/742
good news we were able to remove the backend changes you were concerned about in #64. I should caught this in review

LGTM!

!742 needs to updated against 1.x

This now hard-blocks #3516754: #3516754-24: [PP-1] Finalize how we ascertain a `type: array` (aka multiple-cardinality) SDC/JS prop value is empty.

Wanted to mark this Fixed but spotted the Needs followup tag. Is that still accurate?

Does not need a follow-up see https://git.drupalcode.org/project/canvas/-/merge_requests/567#note_716773 . Was code that was already in 1.x