- Issue created by @narendraR
- First commit to issue fork.
- 🇮🇳India narendraR Jaipur, India
Changes looks good to me, but it needs to be done in different file due to ✨ GUI install multiple modules at once. Needs work . Also instead of patch, it will be good if we use MR.
- 🇸🇬Singapore anish.a Singapore
We are taking up this issue in DrupalCampPune2024
- First commit to issue fork.
- Merge request !604#3476818:Use actual page URL instead of hardcoding the query string in unlock URL. → (Merged) created by utkarsh_33
- 🇺🇸United States phenaproxima Massachusetts
I don't think this actually solves the problem. Here's why.
If the
err.unlock_url
property looks like this:/admin/modules/browse/unlock
, then the current code will generate this:/admin/modules/browse/unlock&destination=my+current+url
. That's an invalid URL which will lead to a 404 or worse.What we need here is:
- If the unlock URL looks something like
/admin/modules/browse/unlock?some_param=foo
, then the generated URL has to look like:/admin/modules/browse/unlock?some_param=foo&destination=my+current+url
. This, to be fair, is what the code in HEAD already does. - But if the unlock URL doesn't have a query string, then we need to append it, prefixed by a question mark.
- If the unlock URL looks something like
- 🇺🇸United States chrisfromredfin Portland, Maine
We have native JS API's for this now, so we don't need to do our own string concat. I think something like this would make the code more readable. (Bear in mind this is GPT-generated and untested, but point being we should use URL - https://developer.mozilla.org/en-US/docs/Web/API/URL )
// Get the current page URL as a URL object for easier manipulation const currentUrl = new URL(window.location.href); if (err.unlock_url) { const unlockUrl = new URL(err.unlock_url); // Append the 'destination' parameter to unlockUrl unlockUrl.searchParams.set('destination', currentUrl.toString()); // Update the HTML content div.innerHTML += `<p>${errorMessage} <a href="${unlockUrl.toString()}">${Drupal.t( 'Unlock Install Stage' )}</a></p>`; } else { div.innerHTML += `<p>${errorMessage}</p>`; }
The failing tests are not related to the changes in this MR, so marking this NR again.
- 🇺🇸United States phenaproxima Massachusetts
Found something that is suspicious at best, wrong at worst, and probably needs test coverage. But I may be wrong. If I am, great - let's just add a comment to clarify and it's full steam ahead.
- 🇺🇸United States phenaproxima Massachusetts
Reviewed. I'm still not sure we are doing this correctly, which is why I think adding some functional JS test coverage is a good idea.
- Status changed to Needs review
28 days ago 9:12am 25 November 2024 @phenaproxima I added functional js tests as you suggested and i think those are taking care of sub directory issue that you were mentioning in this as tests were initially failing here because of the sub directory path was appended correctly in the code but we were not taking that into account for tests.
Marking it NR again.Resolved all the open threads and addressed feedbacks as well.Marking it NR again.
- 🇮🇳India narendraR Jaipur, India
Looks good to me, except some minor suggestions.
-
chrisfromredfin →
committed 17c1f059 on 2.0.x authored by
utkarsh_33 →
Issue #3476818: Use actual page URL instead of hardcoding the query...
-
chrisfromredfin →
committed 17c1f059 on 2.0.x authored by
utkarsh_33 →
Automatically closed - issue fixed for 2 weeks with no activity.