- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I think @mstrelan is right here, we're using the third argument to parseHTML so we're not getting any filtering anyway
- last update
about 1 year ago 29,559 pass - Status changed to Needs review
about 1 year ago 12:51am 27 June 2023 - Status changed to RTBC
about 1 year ago 2:42pm 28 June 2023 - 🇺🇸United States smustgrave
Refactor looks good and applying locally doesn't seem to break anything.
- last update
about 1 year ago 29,563 pass 31:54 30:44 Running- last update
about 1 year ago 29,571 pass - last update
about 1 year ago 29,801 pass - last update
about 1 year ago 29,802 pass - last update
about 1 year ago 29,802 pass - last update
about 1 year ago 29,806 pass - last update
about 1 year ago 29,811 pass - last update
about 1 year ago 29,815 pass - last update
about 1 year ago 29,815 pass - last update
about 1 year ago 29,822 pass - last update
about 1 year ago 29,834 pass, 1 fail - last update
about 1 year ago 29,878 pass - last update
about 1 year ago 29,881 pass - last update
about 1 year ago 29,885 pass - Status changed to Needs review
about 1 year ago 2:05pm 29 July 2023 - 🇫🇮Finland lauriii Finland
Would be great if we could get confirmation from @nod_ or @justafish on this. The
$.parseHtml
is using jQuerys own way of handling fragments which at least provides an API for filtering HTML. We could probably manage this by publishing a CR but it would be good to get confirmation that there aren't any other considerations. - Status changed to Needs work
about 1 year ago 11:49am 8 August 2023 The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- last update
about 1 year ago 29,958 pass - Status changed to Needs review
about 1 year ago 3:44am 9 August 2023 - 🇦🇺Australia mstrelan
I don't understand what the bot wants me to do.
"Patch does not apply"
"This does not mean that the patch needs to be re-rolled or the MR rebased."
If both are true then this should not be "Needs work".
Anyway, I've merged 11.x in to the branch and pushed it back up. Setting back to NR. Updated remaining tasks.
- Status changed to Needs work
about 1 year ago 5:30am 9 August 2023 - 🇫🇷France nod_ Lille
@mstrelan the bot comment is there to discourage credit farming from people from doing rerolls without addressing anything else that needs to be addressed and leave the issue stalled.
Spent some time looking at the replacement, still not sure it's safe. We do keep scripts although the scripts are not executed at parse time, they're executed when we add the elements to the DOM after the parsing. Here they would be executed during the parsing. I'm not convinced it's a problem, especially since we don't add JS with that codepath anymore (since the introduction of the add_js command). So we could try to set keepScripts to false and see what happens with the testbot first. That would be a good indication this is not a concern anymore. In any case the behavior change should be in a change notice somewhere.
If we don't have scripts to take care of, next is prefiltering html as lauriii pointed out. While is not a very scientific way of looking at this, I've never seen it used and Drupal is in charge of making things 'safe' before it gets to the JS so a developer will probably reach out to some PHP before trying to figure out this jquery API.
There is some code related to text nodes parents in jQuery, that may introduce a memory leak. It's over text nodes and Drupal is hardly a single page app when used with the native ajax framework so probably not a huge deal but would be good to get some data on whether this doesn't cause issues with very large text nodes.
- First commit to issue fork.
- 🇮🇳India shubh511
same here as well raised a MR for this issue but the problem is that it is failing pipeline and when fixing one test case will result in failing another test case and viceversa..
- 🇫🇷France nod_ Lille
can someone please try this here?
So we could try to set keepScripts to false and see what happens with the testbot first
- First commit to issue fork.
- 🇷🇺Russia kostyashupenko Omsk
I have used new MR 6957, because 1251 have huge rebase with conflicts (i decided to not waste the time - that's only one reason).
Basically in my commit i was trying to resolve #17 📌 Refactor (if feasible) use of jquery parseHTML function to use vanillaJS Needs work - Status changed to Needs review
6 months ago 10:27am 21 March 2024 - Status changed to RTBC
5 months ago 11:05am 10 April 2024 - 🇲🇩Moldova thebumik
Looks good to me, installed and tested it locally (no issues so far).
Please have @nod_ double-check as well. - 🇮🇳India Pravesh_Poonia
@kostyashupenko Tested Merge request !6957, changes are showing correct
- 🇫🇷France nod_ Lille
Opened a MR to test #17 and #21, we need to make sure we don't need to care about scripts in there.
- 🇫🇷France nod_ Lille
ok so it's just the specific test that fails, not other functionality in core
- Status changed to Needs work
5 months ago 8:57am 11 April 2024 - 🇫🇷France nod_ Lille
So yeah code from !1251 was enough given the way we call this, we don't need the extra options since we always call the function with the same parameters
Security considerations I talked about in #5 don't apply here since we always call the function with the document parameter, and I haven't seen the
htmlPrefilter
"hook" jquery provide used in core/contrib. This is not where security-related things should be happening.As #14 mentioned we could use a change record to explain that htmlPrefilter is not available anymore for ajax content.
- Status changed to Needs review
5 months ago 8:50am 12 April 2024 - 🇮🇳India shubh511
Acknowledged feedbacks, not sure why pipeline failing checks...
- Status changed to Needs work
5 months ago 5:27pm 17 April 2024 - 🇺🇸United States smustgrave
Reran failing javascript test and fails every time
Also was tagged for CR so moving to NW for those.
- Status changed to Needs review
5 months ago 6:25am 19 April 2024 - Status changed to Needs work
5 months ago 1:38pm 24 April 2024 - 🇦🇺Australia mstrelan
Not sure why you keep adding empty comments Pravesh_Poonia.
Also not sure what the CR is for. None of the other jquery removal issues have CRs, and we're not adding or changing any APIs.
- Status changed to Needs review
4 months ago 6:06am 29 April 2024 - Status changed to RTBC
4 months ago 7:33pm 24 May 2024 - 🇺🇸United States smustgrave
Believe the CR was a leftover tag actually so will remove it.
Seems all threads and feedback have been addressed and gone through a few rounds of review. Going to mark it.
- Status changed to Needs work
4 months ago 9:08pm 24 May 2024 - Status changed to Needs review
3 months ago 12:17pm 26 May 2024 - Status changed to RTBC
3 months ago 5:47pm 17 June 2024 - Status changed to Fixed
3 months ago 7:44pm 17 June 2024 - 🇫🇷France nod_ Lille
Committed and pushed b4a0261008 to 11.x and cd700017d1 to 11.0.x and 2d482e076f to 10.4.x. Thanks!
Automatically closed - issue fixed for 2 weeks with no activity.