[5.x] Fix error when augmenting Bard fields #10104

duncanmcclean · 2024-05-17T10:46:20Z

This pull request fixes an issue when augmenting Bard fields with a text set configured.

Edit: My original theory was that the error is caused by a Bard field containing a text set, however that no long seems to be true... not quite sure what the cause is but this PR seems to fix it anyway.

The issue

After some digging, it seems like the error is being caused by writing text directly in a Bard field, while also having a Bard set called text, with a subfield also called text that isn't a Text field.

For example:

You create a Bard field called modules.
Inside it you add a set called text.
Inside that set, you add a Bard field called text
Then, inside your Bard field, you write some text.
On your frontend, when it tries to augment, you see the error.

When augmenting the Bard text, because both the set & the field are called text, it used the Bard fieldtype to try and augment the text:

The "inline" Bard text was being augmented to a Text field (which is correct).

However, in here, another Value object was being created using the Bard fieldtype and the Value object just created for augmenting the Text field was passed in as the "raw" value.

The issue then happened when you tried to use that text field in Antlers, since that's when it'll call the Bard::performAugmentation method, with the Value object passed as the $value.

Previously, before #9198 was merged, it worked by fluke since it'd get caught by the $this->shouldSaveHtml() conditional and just return back the Value object but since we added the is_string conditional, that Value object then made its way down all the Bard augmentation stuff which isn't expecting a Value object.

The fix

This PR fixes the issue by adding a check to Augmentor::augmentSets to catch any text values that have already been wrapped in a Value object.

I've added a test to ensure this edge case gets handled properly and another one to ensure I didn't cause a regression of #9198.

_^ Hopefully this brain dump makes sense. If it doesn't let me know. 😆 _

Closes #9352.

afonic · 2024-05-17T12:41:48Z

Good catch! Just two points:

The "default" Bard set type is text, meaning that if you have a Bard field with some sets and you also write some text outside of the sets, it's treated as a set with type text (I hope that makes sense).
In my case I didn't have a Bard set with a field called text, but this fix also fixes my problem. (probably because of the above point)

duncanmcclean · 2024-05-17T13:28:48Z

The "default" Bard set type is text, meaning that if you have a Bard field with some sets and you also write some text outside of the sets, it's treated as a set with type text (I hope that makes sense).

Yeah, that makes sense.

In my case I didn't have a Bard set with a field called text, but this fix also fixes my problem. (probably because of the above point)

Oh interesting... do you have a set called text though?

afonic · 2024-05-17T13:33:30Z

Oh interesting... do you have a set called text though?

No, but the error occurred on a Bard field that I had content both directly in the editor and in sets. I mix and match them sometimes, probably shouldn't, but it's easier if I have a lot of text with only a couple of sets here and there for rich content.

This check ($set['type'] === 'text' && $set['text'] instanceof Value) is true in my case even though I don't have a set named text, probably because the "raw" text is a set named text.

duncanmcclean · 2024-05-17T13:36:16Z

No, but the error occurred on a Bard field that I had content both directly in the editor and in sets. I mix and match them sometimes, probably shouldn't, but it's easier if I have a lot of text with only a couple of sets here and there for rich content.

Mixing & matching them is fine. I'm just trying to figure out what might be causing the error in the first place if you don't have a text set. 🤔

afonic · 2024-05-17T13:41:36Z

Maybe the fact that the default set is text is enough to trigger it? if you see my post at the issue it was passing a Value instance for some text in the Bard field itself (not inside a set per se, but as a text set).

jasonvarga · 2024-05-17T14:03:41Z

If just having text in a bard field is enough to trigger it then we should be hearing many more reports about this issue. 🤔

afonic · 2024-05-17T14:48:34Z

I am only getting it when using json_encode, not normal Antlers. Whatever the reason it is fixed with this PR through.

jasonvarga · 2024-05-17T21:05:59Z

I am only getting it when using json_encode

Can you show how you are using it?

jasonvarga

The issue stated that the error only started happening since 4.42.1, which points to #9198. So, I think the fix should be something to do with that. The is_string() fails because its a Value instance, even though it would evaluate to a string.

Maybe something like:

protected function performAugmentation($value, $shallow)
{
-    if ($this->shouldSaveHtml() && $isString) {
+    $isString = is_string($value)
+        || ($value instanceof Value && is_string($value->value()));
+
+    if ($this->shouldSaveHtml() && is_string($value)) {

The test you've written does sorta test this, but it's confusing. I can't make sense of this nested mapping and toArray.

$this->assertEquals($expected, collect($augmented)->map->all()->map(fn ($items) => collect($items)->map(fn ($value) => (string) $value))->toArray());

I'd prefer to figure out a way to write a test that better represents what is happening in reality.

afonic · 2024-05-20T07:17:54Z

Can you show how you are using it?

I'm using it in the nav tag to send the navigation to a Vue component.

{{ nav:main include_home="true" as="menuItems" }}
<main-menu
    :items='{{ menuItems | to_json | entities }}'
>
</main-menu>
{{ /nav:main }}

I've narrowed it down to the main Bard content field throwing that error. Note that using the nav tag isn't really the issue here, if I do:

{{ collection:pages as="menuItems" }}
<main-menu
    :items='{{ menuItems | to_json | entities }}'
>
</main-menu>
{{ /collection:pages }}

I get the exact same error. It is caused by empty lines in the Bard field like <p></p> getting parsed as a Value object for some reason.

The Bard field in question is in a fieldset that I then link around in different collections:

title: Content
fields:
  -
    handle: content
    field:
      always_show_set_button: false
      buttons:
        - h2
        - h3
        - bold
        - italic
        - unorderedlist
        - orderedlist
        - removeformat
        - quote
        - anchor
        - image
        - table
      save_html: false
      toolbar_mode: fixed
      link_noopener: false
      link_noreferrer: false
      target_blank: false
      reading_time: false
      fullscreen: true
      allow_source: true
      enable_input_rules: true
      enable_paste_rules: true
      display: Content
      type: bard
      localizable: true
      listable: hidden
      container: assets
      sets:
        main:
          display: Main
          instructions: null
          icon: null
          sets:
            intro_text:
              display: 'Intro text'
              instructions: null
              icon: null
              fields:
                -
                  handle: content
                  field:
                    always_show_set_button: false
                    buttons:
                      - h2
                      - h3
                      - bold
                      - italic
                      - unorderedlist
                      - orderedlist
                      - removeformat
                      - quote
                      - anchor
                      - image
                      - table
                    save_html: false
                    toolbar_mode: fixed
                    link_noopener: false
                    link_noreferrer: false
                    target_blank: false
                    reading_time: false
                    fullscreen: true
                    allow_source: true
                    enable_input_rules: true
                    enable_paste_rules: true
                    display: Content
                    type: bard
                    listable: hidden
                    localizable: false
            text_with_images:
              display: 'Text with images'
              instructions: null
              icon: null
              fields:
                -
                  handle: content
                  field:
                    always_show_set_button: false
                    buttons:
                      - h2
                      - h3
                      - bold
                      - italic
                      - unorderedlist
                      - orderedlist
                      - removeformat
                      - quote
                      - anchor
                      - image
                      - table
                    save_html: false
                    toolbar_mode: fixed
                    link_noopener: false
                    link_noreferrer: false
                    target_blank: false
                    reading_time: false
                    fullscreen: true
                    allow_source: true
                    enable_input_rules: true
                    enable_paste_rules: true
                    display: Content
                    type: bard
                    listable: hidden
                    localizable: false
                -
                  handle: assets
                  field:
                    mode: list
                    container: assets
                    restrict: false
                    allow_uploads: true
                    show_filename: true
                    display: Assets
                    type: assets
                    listable: hidden
                    localizable: false
                -
                  handle: image_position
                  field:
                    options:
                      left: Left
                      right: Right
                    inline: true
                    cast_booleans: false
                    display: 'Image position'
                    type: radio
                    listable: hidden
                    localizable: false
            image_gallery:
              display: 'Image gallery'
              instructions: null
              icon: null
              fields:
                -
                  handle: images
                  field:
                    mode: list
                    restrict: false
                    allow_uploads: true
                    show_filename: true
                    display: Images
                    type: assets
                    listable: hidden
                    instructions_position: above
                    localizable: false
      instructions_position: above
      visibility: visible
      replicator_preview: true
      smart_typography: false
      inline: false
      word_count: false
      remove_empty_nodes: false
      antlers: false
      collapse: false
      previews: true
      hide_display: false

It's doesn't have Save as HTML enabled if that helps.

afonic · 2024-06-15T10:08:54Z

I returned to this site and unfortunately the fix by @jasonvarga doesn't seem to work. The original fix by Duncan does.

Can I somehow help debug this?

duncanmcclean and others added 4 commits May 16, 2024 12:26

Add test to prevent regression of #6045

5688c4f

Add failing test

ccee93f

Catch non-set text fields when augmenting sets

4d7841d

Fix styling

902894b

duncanmcclean mentioned this pull request May 17, 2024

ArrayIterator::__construct(): Argument #1 ($array) must be of type array, string given #9352

Open

duncanmcclean changed the title ~~[5.x] Fix error when augmenting Bard field with text set~~ [5.x] Fix error when augmenting Bard fields May 17, 2024

jasonvarga requested changes May 17, 2024

View reviewed changes

jasonvarga added 2 commits May 28, 2024 15:01

revert test

4adfd67

change implementation with note

f50a626

jasonvarga approved these changes May 28, 2024

View reviewed changes

jasonvarga merged commit efc2c39 into 5.x May 28, 2024
31 checks passed

jasonvarga deleted the fix/bard-augmentation-when-text-set-exists branch May 28, 2024 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.x] Fix error when augmenting Bard fields #10104

[5.x] Fix error when augmenting Bard fields #10104

duncanmcclean commented May 17, 2024 •

edited

Loading

afonic commented May 17, 2024

duncanmcclean commented May 17, 2024

afonic commented May 17, 2024 •

edited

Loading

duncanmcclean commented May 17, 2024 •

edited

Loading

afonic commented May 17, 2024

jasonvarga commented May 17, 2024

afonic commented May 17, 2024

jasonvarga commented May 17, 2024

jasonvarga left a comment

afonic commented May 20, 2024

afonic commented Jun 15, 2024

[5.x] Fix error when augmenting Bard fields #10104

[5.x] Fix error when augmenting Bard fields #10104

Conversation

duncanmcclean commented May 17, 2024 • edited Loading

The issue

The fix

afonic commented May 17, 2024

duncanmcclean commented May 17, 2024

afonic commented May 17, 2024 • edited Loading

duncanmcclean commented May 17, 2024 • edited Loading

afonic commented May 17, 2024

jasonvarga commented May 17, 2024

afonic commented May 17, 2024

jasonvarga commented May 17, 2024

jasonvarga left a comment

Choose a reason for hiding this comment

afonic commented May 20, 2024

afonic commented Jun 15, 2024

duncanmcclean commented May 17, 2024 •

edited

Loading

afonic commented May 17, 2024 •

edited

Loading

duncanmcclean commented May 17, 2024 •

edited

Loading