hyva-themes / magento2-hyva-admin

This module aims to make creating grids and forms in the Magento 2 adminhtml area joyful and fast.
https://hyva-themes.github.io/magento2-hyva-admin/
BSD 3-Clause "New" or "Revised" License
168 stars 39 forks source link

Duplicate Form URL Params Prevent Proper Filtering and Sorting #73

Closed zaximus84 closed 12 months ago

zaximus84 commented 1 year ago

Steps to reproduce: (specific field names and values are just examples)

  1. Create a grid with fields for Year, First Name, and Last Name. Ensure that ajax is enabled.
  2. In the Year filter, enter 2024 and hit enter
  3. After results are refreshed, enter 2023 in the Year filter and hit enter
  4. After results are refreshed, click the First Name column heading to sort by that column
  5. After results are refreshed, click the Last Name column heading to sort by that column

Expected behavior: Results are updated accurately after each filter or sort action.

Actual behavior: Results are only updated after the first instance of an action type. 2024 filters the results, but 2023 shows results that still match 2024. Sort by first name sorts the results, but sort by last name shows results that are still sorted by first name.

Technical Details This seems to be an oddity in PHP / server behavior for duplicate $_GET keys, triggered by the form urls. As you continue interacting with the grid, it doesn't replace the parameters in the ajax url. It adds them to the existing ones. For instance, when you first filter by year = 2024, you see this url is used to refresh content:

https://example.com/backend/hyva-admin/ajax/paging/?ajax=1?gridName=example_example_grid&example_example_grid[_filter][year]=2024

If you then filter by 2023, you see this url:

https://example.com/backend/hyva-admin/ajax/paging/?ajax=1?gridName=example_example_grid&example_example_grid[_filter][year]=2024&example_example_grid[_filter][year]=2023

New filter values are simply appended to the current form action. That action comes from getFilterFormUrl, which uses _current when getting the url to include all current parameters. The result is that the same parameter names are used multiple times with different values, with the latest being the correct value. From my research, PHP will generally just see the last value for any given key, but on our staging server, that's not the case. As a result, the first filter value for a given field is always applied, and the first sort is always applied.

While this could potentially be designated a PHP / server configuration issue (I'm not actually sure what's causing the behavior on our server), the urls really shouldn't have duplicate parameter keys.

Possible Solutions It may be possible to omit the current parameters from the form url (in getFilterFormUrl) as well as the sort urls (getSortByUrl), but I'm not familiar enough with the module architecture to know if that would cause problems. I solved this by patching view/adminhtml/templates/grid.phtml to remove any duplicate keys in the javascript right before the grid is updated:

diff --git a/view/adminhtml/templates/grid.phtml b/view/adminhtml/templates/grid.phtml
--- a/view/adminhtml/templates/grid.phtml
+++ b/view/adminhtml/templates/grid.phtml
@@ -413,6 +413,7 @@ $rows       = $grid->getRows();
             },

             updateGrid(url) {
+                url = this.removeDuplicateUrlParamKeys(url);
                 if (<?= (int) $navigation->isAjaxEnabled() ?>) {
                     this.ajaxGridUpdate(url);
                 } else {
@@ -420,6 +421,19 @@ $rows       = $grid->getRows();
                 }
             },

+            removeDuplicateUrlParamKeys(url) {
+                const urlParts = url.split('?');
+                const baseUrl = urlParts[0];
+                const params = urlParts[1] ? urlParts[1].split('&') : [];
+                const paramsObject = {};
+                // The last value for a given key will overwrite any earlier values
+                params.forEach(function (param) {
+                    const paramParts = param.split('=');
+                    paramsObject[paramParts[0] ?? ''] = param;
+                });
+                return baseUrl + '?' + Object.values(paramsObject).join('&');
+            },
+
             ajaxGridUpdate(url) {
                 this.loading = true;
                 fetch(url, {
Vinai commented 1 year ago

Great find, thanks for reporting!