Overview

Follow-up for #3450303: Implement undo/redo.

When we click on Undo/Redo buttons then we get a 500 error.It's failing on \ComponentElement::generateComponentTemplate(): Argument #4 ($context) must be of type array, null given, called in /var/www/html/web/core/lib/Drupal/Core/Render/Element/ComponentElement.php on line 65 .

Proposed resolution

  • Test coverage
  • Determine root cause.#17
  • Fix UI logic.

User interface changes

Undo/redo behaves as expected.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

hooroomoo created an issue. See original summary.

hooroomoo’s picture

Issue summary: View changes
hooroomoo’s picture

Issue summary: View changes

Utkarsh_33 made their first commit to this issue’s fork.

utkarsh_33’s picture

Title: [PP-1] Fix undo/redo functionality with new Add section feature » Fix undo/redo functionality with new Add section feature
Status: Postponed » Active

I think we are not completely block on Implement add section and add element buttons. We can still use Redo/Undo by adding elements by dragging from the menu bar.

utkarsh_33’s picture

Title: Fix undo/redo functionality with new Add section feature » Fix undo/redo functionality.
Issue summary: View changes

utkarsh_33’s picture

This work is built on top of Implement add section and add element buttons because it's easier to write failing test showcasing that there's a problem with the Undo/Redo button.
I have added the test for undo only for now because i think both are facing the same problem , so fixing one's problem will automatically fox it for he other one.

So now describing the problem faced with this:-
1) When we click on the undo/redo button we get a 500 error.
2) The error prior to the latest 0.x was something like he website encountered an unexpected error. Try again later.<br><br><em class=\"placeholder\">TypeError</em>: Drupal\\Core\\Render\\Element\\ComponentElement::generateComponentTemplate(): Argument #4 ($context) must be of type array, null given, called in /var/www/html/web/core/lib/Drupal/Core/Render/Element/ComponentElement.php on line 65.
3) The error with the latest 0.x is coming out The website encountered an unexpected error. Try again later.<br><br><em class=\"placeholder\">TypeError</em>: array_diff_key(): Argument #1 ($array) must be of type array, null given in <em class=\"placeholder\">array_diff_key()</em> (line <em class=\"placeholder\">36</em> of <em class=\"placeholder\">modules/contrib/experience_builder/src/Controller/SdcController.php</em>).

I also checked the redux states are updated correctly and the model has the what it's supposed to have after clicking undo(one element less that the usual).So i majorly think it's a problem with the backend code(I might be wrong though).

utkarsh_33’s picture

Assigned: utkarsh_33 » Unassigned

The xb-general test fails can be ignored as it's failing on 0.x also.

wim leers’s picture

Status: Active » Needs work

The xb-general test fails can be ignored as it's failing on 0.x also.

They're not: https://git.drupalcode.org/project/experience_builder/-/commit/f2641ed6a... → that's the latest 0.x commit, and it's passing.

utkarsh_33’s picture

The xb-general tests are now passing on this MR.The failing test are to show that existing problem with undo/redo functionality.

bnjmnm’s picture

There are 22 files being changed in the MR and the majority don't appear to be related at all to the reported error. Lets either clean that up or create a new MR.

wim leers’s picture

Title: Fix undo/redo functionality. » Fix undo/redo functionality
Assigned: Unassigned » utkarsh_33

Per @bnjmnm's review in #12.

wim leers’s picture

Title: Fix undo/redo functionality » [PP-1] Fix undo/redo functionality

This work is built on top of Implement add section and add element buttons because it's easier to write failing test showcasing that there's a problem with the Undo/Redo button.

This work/test coverage can continue, but won't be mergeable until #3460952: Implement add button for top level item (section) lands.

wim leers’s picture

Title: [PP-1] Fix undo/redo functionality » Fix undo/redo functionality
Issue tags: +Needs reroll
utkarsh_33’s picture

Assigned: utkarsh_33 » Unassigned
Issue tags: -Needs tests, -Needs reroll

Merged the latest changes from 0.x.The only tests that fails are expected to fail.Removing the tags as tests are already added.

wim leers’s picture

I don't get

\ComponentElement::generateComponentTemplate(): Argument #4 ($context) must be of type array, null given, called in /var/www/html/web/core/lib/Drupal/Core/Render/Element/ComponentElement.php on line 65 

but I do get:

TypeError: array_diff_key(): Argument #1 ($array) must be of type array, null given in array_diff_key() (line 33 of modules/contrib/experience_builder/src/Controller/SdcController.php).
Drupal\experience_builder\Controller\HardcodedPropsComponentTreeItem->resolveComponentProps('7637a2f8-2833-4de3-a4d3-368d413f66d6') (Line: 44)
Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated->getValue() (Line: 82)
Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated->toRenderable() (Line: 295)
Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem->toRenderable() (Line: 342)
Drupal\experience_builder\Controller\SdcController->preview(Object, Object)
…

Possibly because the server-side code has evolved quite a bit.


If I then look at the request that gets sent after clicking Undo, it is:

{
  "layout": {
    "uuid": "root",
    "type": "root",
    "name": "root",
    "children": [
      {
        "uuid": "dynamic-image-udf7d",
        "nodeType": "component",
        "type": "experience_builder:image"
      },
      {
        "uuid": "7637a2f8-2833-4de3-a4d3-368d413f66d6",
        "children": [],
        "nodeType": "component",
        "type": "experience_builder:my-hero"
      },
      {
        "uuid": "static-static-card1ab",
        "nodeType": "component",
        "type": "experience_builder:my-hero"
      },
      {
        "uuid": "dynamic-static-card2df",
        "nodeType": "component",
        "type": "experience_builder:my-hero"
      },
      {
        "uuid": "dynamic-dynamic-card3rr",
        "nodeType": "component",
        "type": "experience_builder:my-hero"
      },
      {
        "uuid": "dynamic-image-static-imageStyle-something7d",
        "nodeType": "component",
        "type": "experience_builder:image"
      }
    ]
  },
  "model": {
    "dynamic-image-udf7d": {
      "image": {
        "src": "public://2024-08/Screenshot 2024-08-06 at 4.56.28 PM.png",
        "alt": "asdf",
        "width": 2560,
        "height": 1414
      },
      "name": "My Test Image Component"
    },
    "static-static-card1ab": {
      "heading": "hello, world!",
      "cta1href": "https://drupal.org",
      "name": "Hero"
    },
    "dynamic-static-card2df": {
      "heading": "sadfsdf",
      "cta1href": "https://drupal.org",
      "name": "Hero"
    },
    "dynamic-dynamic-card3rr": {
      "heading": "sadfsdf",
      "cta1href": "public://2024-08/Screenshot 2024-08-06 at 4.56.28 PM.png",
      "name": "Hero"
    },
    "dynamic-image-static-imageStyle-something7d": {
      "image": {
        "src": "http://core.test/sites/default/files/styles/thumbnail/public/2024-08/Screenshot%202024-08-06%20at%204.56.28%20PM.png.webp?itok=QxxgG3yK",
        "alt": "asdf",
        "width": 100,
        "height": 55
      },
      "name": "My Test Image Component"
    }
  }
}

Note how that second component 7637a2f8-2833-4de3-a4d3-368d413f66d6 is listed in layout, but does not have an entry in model! That's the bug! 🐛

IOW: it makes sense that you get a 500 response (although the server should provide a better error message).

Then if I click "Undo" a second time, it sends:

{
  "layout": {
    "uuid": "root",
    "type": "root",
    "name": "root",
    "children": [
      {
        "uuid": "dynamic-image-udf7d",
        "nodeType": "component",
        "type": "experience_builder:image"
      },
      {
        "uuid": "static-static-card1ab",
        "nodeType": "component",
        "type": "experience_builder:my-hero"
      },
      {
        "uuid": "dynamic-static-card2df",
        "nodeType": "component",
        "type": "experience_builder:my-hero"
      },
      {
        "uuid": "dynamic-dynamic-card3rr",
        "nodeType": "component",
        "type": "experience_builder:my-hero"
      },
      {
        "uuid": "dynamic-image-static-imageStyle-something7d",
        "nodeType": "component",
        "type": "experience_builder:image"
      }
    ]
  },
  "model": {
    "dynamic-image-udf7d": {
      "image": {
        "src": "public://2024-08/Screenshot 2024-08-06 at 4.56.28 PM.png",
        "alt": "asdf",
        "width": 2560,
        "height": 1414
      },
      "name": "My Test Image Component"
    },
    "static-static-card1ab": {
      "heading": "hello, world!",
      "cta1href": "https://drupal.org",
      "name": "Hero"
    },
    "dynamic-static-card2df": {
      "heading": "sadfsdf",
      "cta1href": "https://drupal.org",
      "name": "Hero"
    },
    "dynamic-dynamic-card3rr": {
      "heading": "sadfsdf",
      "cta1href": "public://2024-08/Screenshot 2024-08-06 at 4.56.28 PM.png",
      "name": "Hero"
    },
    "dynamic-image-static-imageStyle-something7d": {
      "image": {
        "src": "http://core.test/sites/default/files/styles/thumbnail/public/2024-08/Screenshot%202024-08-06%20at%204.56.28%20PM.png.webp?itok=QxxgG3yK",
        "alt": "asdf",
        "width": 100,
        "height": 55
      },
      "name": "My Test Image Component"
    }
  }
}

… i.e. now 7637a2f8-2833-4de3-a4d3-368d413f66d6 is gone from layout too, and you no longer get a 500 response. 👍

Conclusion: this is caused by a bug on the client side. The undo/redo history state is incorrect.

wim leers’s picture

I actually bet this is related to and perhaps has even the same root cause as #3452895: Undo/redo - user can undo the loading of the initial state.

wim leers’s picture

Assigned: Unassigned » utkarsh_33
utkarsh_33’s picture

Status: Needs work » Needs review

I was able to fix the issue with the Undo/Redo and i also expanded the test coverage to test both undo and redo button and it works now.Marking it to needs review.

utkarsh_33’s picture

Assigned: utkarsh_33 » Unassigned
utkarsh_33’s picture

I found a small regression issue Undo/redo - user can undo the loading of the initial state (regression). related to the problem being solved in this MR.I think it makes more sense to fix it in this issue only.

utkarsh_33’s picture

Saving credits for @syeda-farheen as Undo/redo - user can undo the loading of the initial state (regression) issue is solved here only.

jessebaker made their first commit to this issue’s fork.

jessebaker’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.