Refactor (if feasible) use of jquery parseHTML function to use vanillaJS

I think @mstrelan is right here, we're using the third argument to parseHTML so we're not getting any filtering anyway

Refactor looks good and applying locally doesn't seem to break anything.

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.

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.

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.

@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.

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..

can someone please try this here?

So we could try to set keepScripts to false and see what happens with the testbot first

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

Looks good to me, installed and tested it locally (no issues so far).
Please have @nod_ double-check as well.

@kostyashupenko Tested Merge request !6957, changes are showing correct

Opened a MR to test #17 and #21, we need to make sure we don't need to care about scripts in there.

ok so it's just the specific test that fails, not other functionality in core

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.

nod_ → closed merge request !7437

Acknowledged feedbacks, not sure why pipeline failing checks...

Reran failing javascript test and fails every time

Also was tagged for CR so moving to NW for those.

It's about the asset size change since the code changed

All test passed...pls review @nod_

Still appears to need a CR per #14 thanks.

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.

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.

Feedback from #30 was not addressed, comments in MR

Believe feedback has been addressed here.

nod_ → closed merge request !6957

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.