- Issue created by @mschudders
- @mschudders opened merge request.
- Status changed to Needs work
over 2 years ago 1:52pm 22 February 2023 The Needs Review Queue Bot โ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide โ to find step-by-step guides for working with issues.
- ๐ง๐ฉBangladesh eashika
enaznin โ made their first commit to this issueโs fork.
- Status changed to Needs review
over 2 years ago 4:39am 27 February 2023 - Status changed to Needs work
over 2 years ago 6:15am 27 February 2023 - ๐ซ๐ทFrance nod_ Lille
There is an
addCommand
method that does much more than a simple array_merge so there is bound to be issues doing it this way.MR should be against 10.1.x since that is the current dev version
Naming of the new function is not ideal. It's not about setting commands, I'd be more of a
mergeCommands
method
At the moment commands can be added the addCommand method, so what the issue raised could be worked around @Mschudders would that be enough? - ๐ง๐ชBelgium mschudders
@nod thanks for the information.
Indeed the naming was crappy.
Created a new branch against 10.1.x with the bot fixes + naming changes to the function.
Let me elaborate why this is usefull:
I have overridden
MediaLibraryWidget
so that I can attach an extra Ajax command to the callback of updateWidget:public static function updateWidget(array $form, FormStateInterface $form_state) { $response = parent::updateWidget($form, $form_state); $lb = new LayoutBuilderExtras(); $responseLbExtraLiveUpdates = $lb->blockAjaxSave($form, $form_state); $commandsLiveUpdate = $responseLbExtraLiveUpdates->getCommands(); $response->mergeCommands($commandsLiveUpdate); return $response; }
Since
updateWidget
returns an AjaxReponse already it isn't easy to add an additional command to it.
If you retrieve the commands from AjaxReponse it's already an rendered array and you cannot set them again. That's why I have added a mergeCommands function so that you can merge the commands.
I hope this clarifies things. - @mschudders opened merge request.
- First commit to issue fork.
- last update
almost 2 years ago 29,420 pass - Status changed to Needs review
almost 2 years ago 10:30am 9 June 2023 - Status changed to Needs work
almost 2 years ago 7:25pm 9 June 2023 - ๐บ๐ธUnited States smustgrave
Can the MR be updated for 11.x as that's the current development branch.
Also could use a simple test assertion to show it's working.
- last update
over 1 year ago 29,644 pass - last update
over 1 year ago 30,341 pass - Merge request !4902Issue #3343670: Allow to merge AjaxCommands inside of AjaxResponse โ (Open) created by tim-diels
- last update
over 1 year ago 30,361 pass - ๐ง๐ชBelgium tim-diels Belgium ๐ง๐ช
The module Layout Builder Extras - live update โ requires this patch.
- First commit to issue fork.
- ๐ง๐ชBelgium kriboogh
Added patch of MR for use in composer.
Applies to both 11.x and 10.3.x - Assigned to shalini_jha
- ๐ฎ๐ณIndia shalini_jha
I have added test coverage for the mergeCommands method, and the pipeline issues have also been resolved.
I am now moving this for review. Please take a look. - ๐บ๐ธUnited States smustgrave
Have not reviewed but this one has multiple branches and multiple MRs up those should be cleaned up or summary updated.
- ๐ฎ๐ณIndia shalini_jha
shalini_jha โ changed the visibility of the branch 3343670-allow-to-merge-ajaxcommands-10-1-x to hidden.
- ๐ฎ๐ณIndia shalini_jha
shalini_jha โ changed the visibility of the branch 3343670-allow-to-merge to hidden.
- ๐ฎ๐ณIndia shalini_jha
I have cleaned up the branch. MR 4902 now contains all required updates and is ready for review. Additionally, I have updated the issue summary for clarity.
Moving this MR to Review. Kindly review the changes. - ๐บ๐ธUnited States smustgrave
This appears to be an API change but that section is missing from the summary. Recommend not removing sections from an issue template even if you aren't sure it applies.
API change will need a change record
- ๐ฎ๐ณIndia shalini_jha
Thank you for your review and feedback; I'll incorporate this. I've updated the issue summary and added a draft mode change request โ for review.
Please take a look and let me know if thereโs anything further needed on my end. - ๐บ๐ธUnited States smustgrave
Tweaked the CR with an example, but rest seems good.
- ๐ฎ๐ณIndia shalini_jha
The method name mergeCommands was chosen to align with the existing structure of the AjaxResponse class, which focuses on handling and manipulating commands, as seen in methods like addCommand and getCommands. Keeping the name consistent with the rest of the class felt logical. However, if thereโs a preference to rename it to mergeResponse for better clarity or to better reflect its purpose, Iโm open to making that change. Let me know your thoughts!
- Issue was unassigned.
- ๐ฌ๐งUnited Kingdom catch
This is requiring that the command is rendered before it's merged in. I would expect you could pass actual commands here, not already rendered commands, to match addCommand(). And even if there's a reason that can't be done, the documentation doesn't specify what the actual contents of the argument is.
Also it will result in the attachments logic in AddCommand not running, and the new test coverage does not cover that - it's not clear at all what will happen to the attachments.
@nod_'s mergeResponse() suggestion might be able to handle it, if you passed in the full AjaxResponse object, and this method then extracted both the rendered commands and the attached from it.
Either that, or I think we need to look at refactoring things a bit more, so that commands are not immediately rendered when added, and so that ::getCommands() or a new getAjaxCommands() method can return command objects.
- ๐ฌ๐งUnited Kingdom catch
The issue summary and change record could use an update here since they no longer reflect the approach in the MR.
- ๐ต๐ฑPoland Graber
graber โ changed the visibility of the branch 3343670-allow-to-merge-ajaxcommands-11-x to hidden.
- ๐ต๐ฑPoland Graber
graber โ changed the visibility of the branch 11.x to hidden.
- ๐ต๐ฑPoland Graber
Updated, including tests a bit as well to illustrate direct usage of
return $response->mergeWith($other);
- ๐บ๐ธUnited States smustgrave
Left a few small comments on the MR
If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
- Assigned to tim-diels
- ๐ง๐ชBelgium tim-diels Belgium ๐ง๐ช
I'll update the code to fix the comments
- ๐ฎ๐ณIndia shalini_jha
Hi, I have re-run the previously failed test and it has now passed. It appears to have been a random failure :)
- ๐ง๐ชBelgium tim-diels Belgium ๐ง๐ช
Ok then we can set this to needs review. Thanks.