Problem/Motivation

The optional <content:encoded> element defines the full content of an <item>. Its value should be either entity-encoded or CDATA-escaped version of the content of the item.

Current format_xml_elements() implementation though does not allow for CDATA-escaped values (or unencoded in general) - they always get entity-encoded.

Resources:

Proposed resolution

Modify format_xml_elements() implementation to allow unencoded values if requested directly (by adding new encoded property to XML element). Keep current behaviour if property is not set to ensure full backwards compatibility.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

maciej.zgadzaj’s picture

Status: Active » Needs review
FileSize
729 bytes

Proposed patch fixing the issue.

mpv’s picture

@maciej.zgadzaj thanks, I like your solution. I'm attaching the patch for drupal 7.x in case anyone finds it useful.

wonder95’s picture

Status: Needs review » Reviewed & tested by the community

I've been using this for 5 months now, and it works perfectly. I'm still re-applying it after every core update for D7, so it would be nice if it could be committed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: drupal-format_xml_elements_unencoded-2393461-1.patch, failed testing.

Andre-B’s picture

#2 works for me

dawehner’s picture

Status: Needs work » Fixed

We killed that in the meantime.

Andre-B’s picture

how and when was this solved for d7?

longwave’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Fixed » Active

Reopening for 7.x, someone should upload #2 as a .patch file for it to be tested.

Sagar Ramgade’s picture

Here's the patch from #2.

Sagar Ramgade’s picture

Status: Active » Needs review
drikc’s picture

Status: Needs review » Reviewed & tested by the community

Successfully skip item value encoding by using the encoded property!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

The patch looks good, but I think the new option definitely needs to be mentioned in the function documentation. (Especially since it has security implications - we need to be really clear in the documentation when it should/shouldn't be used.)

mpv’s picture

Here's the patch with updated funcion documentation.

mpv’s picture

Status: Needs work » Needs review
wonder95’s picture

Status: Needs review » Reviewed & tested by the community

This has been out there for a year and a half, and it now has function documentation as well as a test. Can we please get this committed so that I don't have to manually re-add this patch after every update?

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice
+++ b/includes/common.inc
@@ -1767,9 +1767,13 @@ function format_rss_item($title, $link, $description, $args = array()) {
+ * entity-encoded or CDATA-escaped (see issue #2393461).

We usually don't reference issues in code in D7 - except for @todo's.

Lets also add something like:

"Only use this option if you have already escaped the data as not doing so has security implications."

davic’s picture

Status: Needs work » Needs review
FileSize
1.38 KB

Removed the issue reference as it's not a common use in drupal 7 (see #16) and added the comment suggestion by Fabianx.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice +Pending Drupal 7 commit

RTBC, Stefan will need to commit this.

stefan.r’s picture

Assigned: Unassigned » David_Rothstein

David, are you happy with docs here?

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
FileSize
1.42 KB
777 bytes

Mostly looks good, but the new sentence should be part of the previous paragraph - otherwise it's not clear what it refers to.

Also, if you CDATA-escape untrusted input, that doesn't make it safe (at least not when done naively, and I'm not aware of any function in Drupal or PHP that helps you do CDATA-escaping safely).

So thinking about this a bit more... maybe we should be safe and always just recommend entity-encoding in the case where you're working with untrusted user input? (My understanding is that they are basically interchangeable anyway.)

Like the attached, in other words.

David_Rothstein’s picture

Saving issue credits - I think there is a bug on drupal.org that sometimes makes them get out of whack.

stefan.r’s picture

I think the current wording makes sense...

stefan.r’s picture

Issue tags: +7.50 release notes

  • stefan.r committed 6c75ac1 on 7.x
    Issue #2393461 by David_Rothstein, mpv, maciej.zgadzaj, Sagar Ramgade,...
stefan.r’s picture

Issue tags: -Pending Drupal 7 commit

Committed and pushed to 7.x, thanks!

stefan.r’s picture

Status: Reviewed & tested by the community » Fixed
nevergone’s picture

  • stefan.r committed 86fc546 on 7.x
    Issue #2393461 followup by nevergone: format_xml_elements() does not...
stefan.r’s picture

Thanks @nevergone

Sagar Ramgade’s picture

@stefan just noticed a small typo in the above patch, there are two "a":

+- Added a a new option to format_xml_elements() to allow for already encoded
Sagar Ramgade’s picture

Status: Fixed » Needs review

The last submitted patch, 27: 2393461-wrong-changelog.patch, failed testing.

  • stefan.r committed f68a354 on 7.x
    Issue #2393461 followup by Sagar Ramgade: format_xml_elements() does not...
Sagar Ramgade’s picture

Status: Needs review » Fixed

  • David_Rothstein committed 3705166 on 7.x
    Issue #2393461 CHANGELOG.txt entry should be for Drupal 7.50 (not Drupal...
wonder95’s picture

Thanks for getting this done and committed, everyone. This will be a huge headache saver for me.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.