[PP-1] Make link widget autocomplete work (for uri and uri-reference props)

Created on 13 January 2025, 6 months ago

Overview

SDCs can have a uri or uri-reference prop. For example, my-hero's cta1href prop.

This gets mapped to the link_default widget, which normally within Drupal allows autocompletion on node title to make it easy to link to nodes.

This autocompletion isn't currently working within XB, but it would be nice to make that work.

Fixing this would exercise both integration with autocompletion JS (requiring πŸ“Œ Confirm Semi-coupled form elements can work with autocomplete Active to be solved) and would also be an example of πŸ“Œ Discover cases where is no 1:1 map between field, prop and widget values Active , since the widget value would be the node ID, the field value would be a URI that starts with entity:, and the prop value would need to be the actual URL of the node.

Proposed resolution

User interface changes

✨ Feature request
Status

Postponed

Version

0.0

Component

Page builder

Created by

πŸ‡ΊπŸ‡ΈUnited States effulgentsia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @effulgentsia
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • Status changed to Active 3 months ago
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    The blocker is in

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    πŸ“Œ Allow 6.x version of justinrainbow/json-schema Active is going to be needed here because it uses filter_var to validate URLs and that means entity:node/3 doesn't validate as a valid URI.

    https://github.com/jsonrainbow/json-schema/pull/800 is what fixes this upstream but it's only in the 6.x branch.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    I think I have a way to work around #6 in the meantime

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    πŸ› ComponentValidator ignores the set validator and creates a new one Active prevents us from being able to do this using the APIs provided by justinrainbow/json-schema

  • Merge request !827Issue #3499279 - link autocomplete support β†’ (Merged) created by larowlan
  • Pipeline finished with Canceled
    3 months ago
    Total: 267s
    #461275
  • Pipeline finished with Failed
    3 months ago
    Total: 1715s
    #461276
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    This is blocked on πŸ“Œ Split model values into resolved and raw Active , πŸ› "There are no users matching '' error message appears after reloading the editor page Active and πŸ› ComponentValidator ignores the set validator and creates a new one Active

    Pushed some code that includes a cherry-pick (Squashed) of πŸ“Œ Split model values into resolved and raw Active and πŸ› "There are no users matching '' error message appears after reloading the editor page Active that is working on the backend if the core patch from πŸ› ComponentValidator ignores the set validator and creates a new one Active is applied

    On the frontend - current status is the autocomplete value (e.g Some node title (3) doesn't validate with ajv as a uri and hence the field is invalid. I have written a transform to translate this to a uri (entity:node/1) but transforms aren't applied until after the field value validates so we might need to revisit that. Started looking at ajv keywords which are a schema extension that adds support for transforms - but these don't work on strings, only on nested data/objects (because the string is passed by value when we call jsonSchemaValidate)

  • Pipeline finished with Failed
    3 months ago
    #462955
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    πŸ› "There are no users matching '' error message appears after reloading the editor page Active landed!

    Reviewed. AFAICT this is blocked on πŸ› ComponentValidator ignores the set validator and creates a new one Active shipping in a release of Drupal core ( https://www.drupal.org/project/drupal/releases/11.1.6 β†’ does not yet contain it), and bumping XB's core requirement to that core version.

  • Pipeline finished with Failed
    3 months ago
    Total: 1782s
    #470019
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    This is postponed on waiting for core to tag 11.1.7

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    11.1.7 was released.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 628s
    #494555
  • Pipeline finished with Failed
    about 2 months ago
    Total: 411s
    #494558
  • Pipeline finished with Failed
    about 2 months ago
    Total: 639s
    #494559
  • Pipeline finished with Failed
    about 2 months ago
    Total: 369s
    #494563
  • Pipeline finished with Failed
    about 2 months ago
    Total: 634s
    #494565
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    No real complaints from a BE POV β€” self-fixed the high-level docs remarks.

    I'd like @bnjmnm to review the FE + docs in detail though.

    P.S.: autocomplete-props.cy.js was failing, re-tested: https://git.drupalcode.org/project/experience_builder/-/jobs/5229137

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    P.S.: autocomplete-props.cy.js was failing, re-tested: https://git.drupalcode.org/project/experience_builder/-/jobs/5229137

    saw that happen, did some stability changes, did some repeat runs locally and it looks ok 🀞

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Nope still failed on CI - might be because of the subdir setup on CI, pushed something else

  • πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

    How much of the current MR is due to πŸ“Œ Allow 6.x version of justinrainbow/json-schema Active not yet being in? It's mentioned in a few areas
    but I'm not 100% what would be different once this is in.

    The main thing I'm wondering is, once that lands, would we still need to define the format in the front end? If that's the case there should be explicit explanations of when that's necessary vs the established approach of using the schema from the BE-returned component prop definition. The BE was previously the source of schema truth and I'd like to know when/why that should be deviated from. It's possible this is info is implicit in the docs already in the MR, but I couldn't parse with certainty.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Next step is approval from Ben

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    cross post

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    The main thing I'm wondering is, once that lands, would we still need to define the format in the front end? If that's the case there should be explicit explanations of when that's necessary vs the established approach of using the schema from the BE-returned component prop definition. The BE was previously the source of schema truth and I'd like to know when/why that should be deviated from. It's possible this is info is implicit in the docs already in the MR, but I couldn't parse with certainty.

    No, that won't change this. The move to v6 will allow us to drop all of the service provider shenanigans because v6 correctly sees entity:node/1 as a valid URI whilst v5 does not.

    In terms of the FE, validation runs before transforms so we need a way to tell ajv that `This is a node title (3)` is valid before then transforming it to entity:node/3

    Will fix that export/import

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    I've used the import from Ajv, much cleaner - and added some more inline comments to clarify why it is needed per #20

    Thanks for the review!

  • Pipeline finished with Success
    about 2 months ago
    Total: 2751s
    #496494
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    The main thing I'm wondering is, once that lands, would we still need to define the format in the front end? If that's the case there should be explicit explanations of when that's necessary vs […]

    πŸ’― β€” yes, totally, because this is really a non-trivial work-around to something that a combination of upstream dependencies (Drupal core, and even Composer IIRC!) is forcing upon us! πŸ€ͺ

    Alas, @larowlan indicated in #23 that that updated upstream dependency won't change all that much for us, except for the provider πŸ€·β€β™€οΈ

    Signaling that @bnjmnm should feel free to merge this when he considers it ready: RTBC! πŸ˜„

  • Pipeline finished with Failed
    27 days ago
    Total: 786s
    #514722
  • Pipeline finished with Canceled
    27 days ago
    Total: 122s
    #514760
  • Pipeline finished with Failed
    27 days ago
    Total: 995s
    #514764
  • Assigned to larowlan
  • Status changed to Needs work 15 days ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Un-RTBC'ing a ~month after #17 and #25.πŸ˜‡

    Reasons:

    1. How is this going to work for a code component, which cannot use the link Twig function? That uses the \Drupal\Core\Template\TwigExtension::getLink() server-side logic, which is impossible to replicate on the client side. (Drupal.url() doesn't provide equivalent functionality.)
    2. see this comment on the MR. 
    3. This is missing test coverage for explicitly distinguishing between format: uri and format: uri+entity_autocomplete, i.e. something like:
          test_string_format_uri_drupal:
            title: 'String, format=uri+entity_autocomplete'
            type: string
            format: uri+entity_autocomplete
            examples:
              - entity:node/1
      

      in addition to the current

          test_string_format_uri:
            title: 'String, format=uri'
            type: string
            format: uri
            examples:
              - https://uri.example.com
      

      in all-props.component.yml

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    transforms aren't applied until after the field value validates so we might need to revisit that.

    Yes, I think we need to revisit this. I'm actually surprised we've managed to get this far with XB with this existing order of operations. It seems fundamentally incorrect. We're using AJV to validate, which means our validation is based on the json schema, which means the thing we need to validate is the prop value, but we don't have a prop value until we run the transforms. Prior to running the transforms, we don't have a prop value, we only have a form input value, and form input values are allowed to be anything so long as the transforms turn them into a valid prop value.

    Should we open a separate issue to fix this, or do it as part of this issue, since this issue is the first to surface the problem?

  • Pipeline finished with Failed
    14 days ago
    Total: 842s
    #524936
  • Pipeline finished with Failed
    14 days ago
    Total: 1135s
    #524945
  • Pipeline finished with Failed
    14 days ago
    Total: 1436s
    #524962
  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia
  • Pipeline finished with Canceled
    13 days ago
    Total: 134s
    #525703
  • Pipeline finished with Failed
    13 days ago
    Total: 1071s
    #525704
  • Pipeline finished with Failed
    13 days ago
    Total: 1223s
    #525733
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Ready for review again

  • Pipeline finished with Success
    13 days ago
    Total: 1838s
    #525755
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Big leap forward! πŸ‘

    Addresses many concerns I previously raised in #26. Still some Qs on the MR though.

    I'd like @bnjmnm to review the client-side transforms changes. πŸ™

  • πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

    Yes, I think we need to revisit this. I'm actually surprised we've managed to get this far with XB with this existing order of operations. It seems fundamentally incorrect. We're using AJV to validate, which means our validation is based on the json schema, which means the thing we need to validate is the prop value, but we don't have a prop value until we run the transforms. Prior to running the transforms, we don't have a prop value, we only have a form input value, and form input values are allowed to be anything so long as the transforms turn them into a valid prop value.

    The transformed versions are getting validated, but they're still valid. After digging into the implementations, the format requirements are more relaxed than I'd assumed.

    If it is validating entity:node/2

    • βœ…format: uri passes /^(?:[a-z][a-z0-9+\-.]*:)(?:\/?\/)?[^\s]*$/i (the colon indicates a scheme and the slash indicates a path)
    • βœ…format: uri-reference passes /^(?:(?:[a-z][a-z0-9+\-.]*:)?\/?\/)?(?:[^\\\s#][^\s#]*)?(?:#[^\\\s]*)?$/i (uri-reference is even more forgiving)

    This could be made more robust with the use of pattern: but that alone would result in the scenario mentioned by @effulgentsia where the AJV validation does fail

  • Pipeline finished with Canceled
    about 22 hours ago
    Total: 229s
    #535633
  • Pipeline finished with Failed
    about 22 hours ago
    Total: 664s
    #535635
  • Pipeline finished with Failed
    about 21 hours ago
    Total: 444s
    #535655
  • Pipeline finished with Success
    about 21 hours ago
    Total: 1159s
    #535658
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Ready for hopefully final review by @bnjmnm :)

    Upon re-reviewing this ~10 days since my last review at #31, spotted one more bit (strindata)

  • Pipeline finished with Success
    about 8 hours ago
    #536164
  • Pipeline finished with Success
    about 4 hours ago
    Total: 722s
    #536396
  • Pipeline finished with Skipped
    about 4 hours ago
    #536403
  • Pipeline finished with Skipped
    about 4 hours ago
    #536404
  • πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

    After debugging a bit with the ref-stored transforms I'm more glad it's in there and it might spare us some future headaches.

    I think the back and front end scrutiny has been duly applied, and the changes are good ones, so this shall be merged.

Production build 0.71.5 2024