oEmbed system can't parse XML attributes

Looks good and works well, but we need tests.

It turns out that our existing tests were already perfectly poised to catch this bug. Here ya go!

Works great and tests fail pass as they should, RTBC!

+++ b/core/modules/media/src/OEmbed/ResourceFetcher.php
@@ -194,4 +194,23 @@ protected function createResource(array $data, $url) {
+    // XML resources may have a 'type' attribute in the version tag. In this
+    // case, Symfony's XML parser will interpret the version tag as an array
+    // whose text content is contained in the '#' key.

can you point me to some docs about this?

Unfortunately not; the behavior is undocumented as far as I can tell. Here's the offending line from Symfony's XmlEncoder::decode():

        foreach ($rootNode->attributes as $attrKey => $attr) {
            $data['@'.$attrKey] = $attr->nodeValue;
        }

// BEHOLD!
        $data['#'] = $rootNode->nodeValue;

        return $data;

The only condition this is hidden behind is that the text value of the node is returned directly if, and only if, there are no attributes. If there are attributes, this is what we get. Sucks that it's not documented, though!

What about if type or some other attribute is added to the title element in the XML? Do we need a more generic thing to normalise $data?

Step debugged through the Symfony code. The code that's responsible for this in our case is:

    /**
     * Parse the input DOMNode into an array or a string.
     *
     * @return array|string
     */
    private function parseXml(\DOMNode $node, array $context = array())
    {
        $data = $this->parseXmlAttributes($node, $context);

        $value = $this->parseXmlValue($node, $context);

        if (!\count($data)) {
            return $value;
        }

        if (!\is_array($value)) {
            $data['#'] = $value;

            return $data;
        }

As far as I can see there is no way around this. And if we continue to use XMLEncoder we're going to be fragile if any oembed provider adds an attribute to any element.

I think what we should do here is to commit a workaround here but not do it as we've done here. It should be an ugly hack that does not add a protected method so there is no API change at all and we should open a follow-up to harden this code or use a different way to get XML into an array.

The hack could look something like this:

    if (strstr($format, 'text/xml') || strstr($format, 'application/xml')) {
      $encoder = new XmlEncoder();
      $data = $encoder->decode($content, 'xml');
      // @todo document...
      if (is_array($data['version'])) {
        $data['version'] = $data['version']['#'];
      }
    }

in \Drupal\media\OEmbed\ResourceFetcher::fetchResource()

Alternatively if we want to fix this reliably here we could do something like

    if (strstr($format, 'text/xml') || strstr($format, 'application/xml')) {
      // Convert XML to JSON for consistent array structures regardless of
      // XML attributes.
      $internalErrors = libxml_use_internal_errors(true);
      libxml_clear_errors();
      $content = simplexml_load_string($content, "SimpleXMLElement", LIBXML_NOCDATA);
      libxml_use_internal_errors($internalErrors);
      if ($error = libxml_get_last_error()) {
        libxml_clear_errors();
        throw new ResourceException($error->message, $url);
      }
      if ($content === FALSE) {
        throw new ResourceException('The fetched resource XML could not be parsed.', $url);
      }
      $data = Json::decode(json_encode($content));
    }

... well probably move that into a protected method in \Drupal\media\OEmbed\ResourceFetcher()

I like the second approach better. This also explains why an older iteration of the oEmbed system did exactly that -- convert XML to JSON and then back again. I didn't know why it did that previously (there was no comment about it) so I removed it. Now it's bitten us in the butt, so I'll restore it in a protected method. Patch forthcoming today.

How's this look?

1. +++ b/core/modules/media/src/OEmbed/ResourceFetcher.php
   @@ -116,7 +114,7 @@ protected function createResource(array $data, $url) {
   -    if ($data['version'] !== '1.0') {
   +    if (!$this->verifyResourceVersion($data['version'])) {
   

   This will fail.

2. +++ b/core/modules/media/src/OEmbed/ResourceFetcher.php
   @@ -194,4 +192,42 @@ protected function createResource(array $data, $url) {
   +    $data = json_encode($content);
   +    return Json::decode($data);
   

   This looks weird... either we do json_encode()/json_decode() or Json::encode()/Json::decode() but mising them feels awkward. Let's go with Json:: as that's what we're using for the json variant.

Ugh, sorry. Didn't have enough of a caffeine jolt this morning. All fixed.

Hey look, I'm still a moron.

Committed and pushed 6ca0cd27aa to 8.7.x and faa28e8e08 to 8.6.x. Thanks!