Surface the REASON for an SDC not being made available in XB (i.e. not meeting criteria)

Wim Leers → credited f.mazeikis → .

tests/src/Kernel/ComponentTest.php will need to have the

…
        'sdc_test:my-banner' => TRUE,
        'xb_test_sdc:props-no-slots' => FALSE,
        'xb_test_sdc:props-slots' => FALSE,
…

expectations updated to something like:

…
        'sdc_test:my-banner' => 'fails requirement FOO',
        'xb_test_sdc:props-no-slots' => 'fails requirement BAR',
        'xb_test_sdc:props-slots' => 'fails requirement BAZ',
…

This is in the critical path for 🌱 Milestone 0.1.0: Experience Builder Demo Active — see #3454125-53: Implement temporary design system for the DrupalCon Barcelona demo → .1.2.

This will be fantastic 🙏

FYI: @f.mazeikis landed 🐛 Component config entities are incomplete: missing entries for optional props, causing errors in ComponentPropsForm Fixed earlier today, he's now pushing this forward 👍

Pushed the changes so far, don't want to polish this too much before we agree it's going in the right direction.

Definitely on the right track! 🤩👍

One post-review thought: we need to verify that the state service remains in sync with the SDC discovery cache. Otherwise, we risk that the state key-value pair is deleted while the SDCs already are discovered. That'd result in zero incompatible SDCs getting listed, while we know that's not the case.

This blocks 📌 [PP-2] Add ComponentAuditabilityTest Active .

I think I've addressed all of the feedback.

This is so great! Tested the MR branch and one thing that seems to be wrong is that if you disable=>enable a component then it still shows up in the disabled list. Should it be removed? You can see in the video:

https://youtu.be/5xsS7enWvSA

Issue summary

1. ✅ tests now assert the expected reason for an SDC being incompatible → no more
2. 🙏
   

   Bonus idea: a "Refresh" button that calls ::clearCachedDefinitions() + ::getDefinitions(), to force a refresh :)

   @f.mazeikis, you haven't mentioned this at all, I think it'd take you <5 minutes to implement this and it'd help https://www.drupal.org/project/demo_design_system a lot? 😊🤞

3. 🙏 tag: Screenshots are missing from the issue summary.

Review

I know that you've chosen the pragmatic route to unblock/accelerate @kristen pol ASAP. And I support that.

But then we need to explicitly acknowledge/document those pragmatic choices that we already know are not the right long-term ones. IOW: I'm missing many documentation bits and many @todos. See review threads on the MR for details.

Finally: I previously already said the "reason" strings should be optimized for actionability. It's far better now, but this is still a problem:

👆 That does not match the contents of the *.component.yml files due to different encoding — in the YAML files it is:

      $ref: json-schema-definitions://experience_builder.module/shoe-icon

So let's ensure those schema definition URIs can be copy/pasted and grepped 🙏

WRT my remark on the MR about @state possibly getting wiped: that possibility for inconsistent state is already demonstrated by @kristen pol in https://youtu.be/5xsS7enWvSA?feature=shared&t=36 AFAICT… 🙈

#15: hah, I think I predicted that bug while reviewing the code: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... 😅

Bonus idea: a "Refresh" button that calls ::clearCachedDefinitions() + ::getDefinitions(), to force a refresh :)

This already happens on every page load for the "Disabled components" page. Would you prefer that not to happen and introduce a separate "refresh" button to trigger this behaviour manually?
Other than that, I've addressed (or at least replied to) the feedback on the MR.

#20: oh, I see, I missed that! That's probably better still 👍 We'll have to wait and see if this reveals performance issues down the line.

@f.mazeikis posted screenshots in the MR on Friday, I missed those: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

This MR still has merge conflicts that need to be resolved, plus there are some loose ends that should be addressed prior to merging 🙏 (See detailed feedback on MR.)

Addressed additional feedback, waiting on more feedback on 2 unresolved threads in MR.

The problems/challenges that were identified while working on this and that were discussed should be captured both as:

1. actually existing follow-up issues in the issue queue, because it A) creates a backlog, B) helps get a sense of remaining work, C) empowers new contributors to start contributing on less complicated issues
2. @todos in the codebase pointing to those follow-up issues

Once that's done, this will be RTBC :)

Follow-ups exist now:

1. 📌 XB Component config entity's `status` may only be `true` iff it meets all of XB's requirements Active
2. 📌 Capture all reasons why particular SDC is incompatible Active

👍

Posted one last round of review, found some nits/incorrect suggestion in the *.schema.yml, and a bit of dead code. Once addressed, this will be RTBC 😊

Applied suggestions, converted controller into a service, added comments.

On the verge of landing 🥳 — slightly delayed because @f.mazeikis decided to actually implement my very optional suggestion of using invokable services as controllers → , which does make for a nicer, cleaner end result, and it'll be a good pattern for other controllers to copy 👍

🚀

And updating the issue summary to simplify future git archeology: visualize the change + capture what the decisions ended up being :)

• This helps/accelerates 📌 Implement temporary design system for the DrupalCon Barcelona demo Needs work .
• This unblocks 📌 [PP-2] Add ComponentAuditabilityTest Active .
• And, we could start working on the two follow-ups mentioned in #24 at any time now 👍