- 🇳🇱Netherlands SpadXIII
As the ticket responsible for postponing this has been merged and fixed (and released), I've re-rolled the latest patch against 2.x.
And putting it back to needs review
- 🇩🇪Germany Anybody Porta Westfalica
@SpadXIII thanks! Why are you adding
Markup::create()
? Doesn't that introduce security risks?
It wasn't there before, so I don't know yet, why it should be needed?BTW, Tests are failing, but that might be unrelated?
- 🇳🇱Netherlands SpadXIII
I seem to have forgotten to add a use-statement. Fixed in new patch.
- First commit to issue fork.
- Status changed to Needs work
over 1 year ago 8:26am 25 March 2023 - 🇮🇳India Rajeshreeputra Pune
Agree with @Anybody Markup::create() isn't required.
- First commit to issue fork.
- @rpayanm opened merge request.
- Status changed to Needs review
over 1 year ago 3:18pm 25 March 2023 - Status changed to Needs work
over 1 year ago 3:42pm 25 March 2023 - 🇳🇱Netherlands Tess Bakker
Could use a test to validate the case in comment #7 for double escaping.
- First commit to issue fork.
- 🇮🇳India kunal_sahu Karnataka
Please review the MR, I have tried to write some addition test cases as per @Tess Bakker requirement.
Thanks
- Status changed to Needs review
over 1 year ago 8:05pm 25 March 2023 - last update
6 months ago Patch Failed to Apply - 🇩🇪Germany stefan.korn Jossgrund
I am a bit confused about the status of this issue.
Imho patch from #14 was the best solution so far, maintainer just requesting to not use Markup::create.
Merge Request #40 seems a bit of a mess now, i. e. what is "commerce_product_ui_product_form" doing in there?
I am providing patch based on #14 without Markup::create to maybe get this going again.
Regarding tests: Not sure what should be tested and how and if additional test is really necessary.
- 🇩🇪Germany stefan.korn Jossgrund
@Anybody: Okay, I am not sure about the double escaping. So we would need the Markup::create still to resolve this?
And changing to 2.x branch as this is current.
- last update
6 months ago Patch Failed to Apply - last update
6 months ago Patch Failed to Apply - 🇩🇪Germany stefan.korn Jossgrund
So testing this, it seems without Markup::create special characters in the title will indeed be double escaped. This would mean #15 is still the way to go.
- last update
6 months ago 30 pass - 🇩🇪Germany stefan.korn Jossgrund
now this patch based on #15 but applying to 2.x branch.
- 🇩🇪Germany Anybody Porta Westfalica
@stefan.korn thanks, seems you're right. Guess that happens because of https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Rend...
Perhaps we can find a pattern in core for this case? Otherwise we need to find out, if we introduce security risks here and if yes, which. Perhaps they can be mitigated by using a different method? (If needed). Sorry I'm unsure myself.