Remove format_xml_elements()

Don't we have an issue to remove format_xml_elements() already? Its not really such an helpful API and note: we do have no use for it anymore in core.

Other than format_xml_elements calling itself, the only place I could find a usage (outside of tests) was in Views. Specifically: template_preprocess_views_view_rss() and template_preprocess_views_view_row_rss(). My take is those should be handled in the template and not preprocessed. See also #2337747: HTML entities/tags in the item title are double encoded for feeds using fields.

As @dawehner said in #3, we should probably remove this function and move it's processing to the two relevant Views templates.

I'll look through the tests and see if there's anything lurking there...

Attached patch cleans up Views' RSS formatting and removes the last call to format_xml_element in core outside of tests. I haven't touched format_xml_elements() yet -- wanted to see what the testbot thinks first.

If the testbot comes back green, I'll go ahead and start removing format_xml_elements from the tests and replacing it with the actual XML that should be generated in those tests.

I welcome any feedback on this approach -- I've never worked on removing things from core before... :)

Issue consolidation: Adds the test from #2337747-12: HTML entities/tags in the item title are double encoded for feeds using fields which I'm marking as a dup of this issue as the underlying fix will take care of both.

Also, what idiot marked this as related to itself in #6? /me: ducks out of sight...

+1 to inline the XHML, at remove the usage of format_xml_elements().

Do we consider format_xml_elements() as something removable at this point?

@dawehner: I haven't been able to find any previous discussion about removing format_xml_elements(). Would love to hear what others think about removing it...

I'm for it!

Though we may need to check on contrib usage at beta stage but at the least we could deprecate it.

For what it's worth this, for D7, shows only six calls, all within core and Views. The list of modules tracked is by no means exhaustive, but it's not insubstantial either.

I would guess there would be minimal impact on contrib. Regardless, this would need a change record.

Rerolled against latest HEAD.

Since #2337747: HTML entities/tags in the item title are double encoded for feeds using fields was bumped to Major in comment #3 and then closed as a dup of this issue, I'm bumping this to Major as well.

Ack, totally goofed the reroll in #14. Rerolled the reroll and removed field_langcode field_langcode_add_to_query from the test view as per this CR.

Needs a reroll.

$ git apply 2296885-17-remove-format_xml_elements.patch 
error: patch failed: core/modules/views/src/Tests/Plugin/DisplayFeedTest.php:74
error: core/modules/views/src/Tests/Plugin/DisplayFeedTest.php: patch does not apply
warning: core/modules/views/views.theme.inc has type 100755, expected 100644
error: patch failed: core/modules/views/views.theme.inc:912
error: core/modules/views/views.theme.inc: patch does not apply

Rerolled.

Also realized that I never actually removed format_xml_elements from common.inc. This patch does that as well.

Updated proposed resolution and remaining tasks in the issue summary.

Added beta eval.

In re-reading this issue, it looks like I was waiting for... something... before removing format_xml_elements. The attached patch removes the code and fixes the tests that were relying on it.

In the CR, we will need to mention that contrib modules subclassing the Views RSS style plugin will need to tweak the output of getChannelElements to return a render array instead of an array to be consumed by format_xml_elements.

Fixed HTML in beta eval, tweaking wording in "disruption" section.

Since the testbot came back green after #28.

Patch still applies cleanly -- rerunning tests, just in case.

Patch applies cleanly to today's HEAD and the testbot is green as of 27 April.

Sorry, forgot to unassign myself and remove the reroll tag.

Heh... Also forgot that I started a draft CR about a month ago:

https://www.drupal.org/node/2468139

I'd personally be OK with removing format_xml_elements(), the chances of that being used anywhere are really minimal and I think it does more harm staying in since it's a red herring compared to serializer etc. Could use a second opinion from another committer though, because I do love removing things.

The issue for refactoring Views RSS is #2003108: Switch Views RSS to use Serializer/Zend by the way.

@catch: Agreed, chances for disruption are minimal and the ability to override the default RSS markup is nice. Thanks for the link to the related issue, I've added a link to this issue over there.

+1 to remove it. Xml is less common than before, a template also works pretty good.
In case you want to, there is also tools like https://github.com/nb/oxymel and https://github.com/iwyg/xmlbuilder to make xml building really easy

Rerolled and fixed tests. #2424533: Copy views templates to Classy added views-view-row-rss.html.twig (which is changed in this patch) to Classy. However there are no classes added by this template (it's an RSS feed, after all) so I removed it as part of this patch. See the fixes-tests interdiff.

There is also a minor doc change as shown in the docs-update interdiff.

I totally agree that there is no reason to have that template as part of classy.

http://www.drupalcontrib.org/api/drupal/drupal%21includes%21common.inc/f... shows that in Drupal 7 only views and core used the method I agree that removing this makes sense.

I don't think this issue should remove core/themes/classy/templates/views/views-view-row-rss.html.twig since that is out of scope. Having the templates all in classy is not just about the templates using a class. It is about making things very easy to find.

#41: Makes sense.

This patch updates the template in classy to use the same markup as that in core/modules/views/templates/views-view-row-rss.html.twig.

This patch updates the template in classy to use the same markup as that in core/modules/views/templates/views-view-row-rss.html.twig.

+1

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed cc55e5c and pushed to 8.0.x. Thanks!

+++ b/core/modules/system/src/Tests/Ajax/DialogTest.php
@@ -174,6 +174,7 @@ public function testDialog() {
+

Unrelated change - removed on commit.

The change record here was still in "Draft" state. I went ahead and published it now.