This is a follow-up to #666412-45: Regression: RSS feed enclosure support lost from core, which somehow got missed, as far as I can see. The RSS enclosure formatter supports files in file fields being used for the enclosure element of RSS feeds, but not image fields.

xjm had suggested this could be done as a simple follow-up and/or in contrib. As far as I can see, the state of contrib for this isn't great, and it's such a simple thing to do, that I figured I'd just go do it.

Patch to follow before long.

Issue fork drupal-3150318

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

james.williams created an issue. See original summary.

james.williams’s picture

Assigned: james.williams » Unassigned
Status: Active » Needs review
FileSize
3.39 KB

Here's an attempt, with a test that is based on the one for regular file fields (FileFieldRSSContentTest).

james.williams’s picture

I should also have included a test-only patch demonstrating the issue (which is expected to fail, as the RSS enclosure formatter isn't available for an image field without the addition in image.module).

Status: Needs review » Needs work
james.williams’s picture

Status: Needs work » Needs review

Perfect, that was as expected - the test-only patch failed, but the fix+test passed.

cburschka’s picture

Status: Needs review » Needs work

Patch looks good, and the test definitely verifies that the image field formatter can be set to RSS enclosure in the form.

But the XPath assertion at the end there does not look correct, and equally the assertion in FileFieldRSSContentTest which it duplicates from #2870448: Convert web tests to browser tests for file module.

+++ b/core/modules/image/tests/src/Functional/ImageFieldRSSContentTest.php
@@ -0,0 +1,81 @@
+    $this->assertNotNull($this->getSession()->getDriver()->find('xpath', $selector), 'Image field RSS enclosure is displayed when viewing the RSS feed.');

This calls \Behat\Mink\Driver\CoreDriver::find() with two string arguments, being 'xpath' and an XPath selector. However, the signature of that function has only one argument, being the XPath selector itself.

    public function find($xpath)
    {
        $elements = array();

        foreach ($this->findElementXpaths($xpath) as $xpath) {
            $elements[] = new NodeElement($xpath, $this->session);
        }

        return $elements;
    }

Also it returns an array in all cases, which is empty if no results were found. An empty array will satisfy the assertNotNull, meaning that this assert doesn't test anything.

(The find('xpath', $xpath_selector) signature is intended for $this->getSession->getPage()->find(), not $this->getSession->getDriver()->find(). The former also does return NULL when it doesn't find the element, while the latter returns the empty array.)

The correct line would probably be

    $this->assertNotEmpty($this->getSession()->getDriver()->find($selector), 'Image field RSS enclosure is displayed when viewing the RSS feed.');

or

    $this->assertNotNull($this->getSession()->getPage()->find('xpath', $selector), 'Image field RSS enclosure is displayed when viewing the RSS feed.');

We also have a BrowserTestBase::xpath() function which takes over the sprintf functionality, so the whole thing could be shortened to

    $elements = $this->xpath(
      'enclosure[@url=":url"][@length=":length"][@type=":type"]',
      [
        ':url'    => file_create_url("public://$uploaded_filename", ['absolute' => TRUE]),
        ':length' => $node_file->getSize(),
        ':type'   => $node_file->getMimeType(),
      ]
    );
    $this->assertNotEmpty($elements, 'Image field RSS enclosure is displayed when viewing the RSS feed.');

But that last one's subjective.

james.williams’s picture

Thanks for the review! It does look like that FileFieldRSSContentTest test needs some updating - I just noticed the file_create_url() call has an extra options parameter which isn't used.

I've gone with your last suggestion, I like that.

Status: Needs review » Needs work

The last submitted patch, 7: image-fields-rss-formatter-3150318-7.patch, failed testing. View results

cburschka’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
1.42 KB

Apparently ::xpath()/::buildXPathQuery() automatically encloses arguments in double-quotes.

I've also simplified this part:

+++ b/core/modules/image/tests/src/Functional/ImageFieldRSSContentTest.php
@@ -0,0 +1,83 @@
+    $uploaded_filename = str_replace('public://', '', $node_file->getFileUri());
...
+        ':url' => file_create_url("public://$uploaded_filename"),

since it looks like $uploaded_filename isn't used anywhere else

Status: Needs review » Needs work

The last submitted patch, 9: 3150318-9.patch, failed testing. View results

cburschka’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
1.03 KB

I've checked a few other examples of XPath assertions, and it looks like the syntax here is still wrong... this really shouldn't have been added back in #2870448: Convert web tests to browser tests for file module. :P

I've altered the syntax but I can't yet get it to pass locally. There might be another underlying issue here.

cburschka’s picture

Oops - my suggestion to use BrowserTestBase::xpath() was incorrect. Maybe this doesn't work on non-HTML documents? In any case,
even a simple xpath('//*') fails to find any elements on the rss.xml page.

Switching back to the DriverInterface::find() method (while leaving in place the modifications to the XPath selector and the assertion) makes the test pass again.

The last submitted patch, 11: 3150318-11.patch, failed testing. View results

cburschka’s picture

I filed a separate issue for the fix to FileFieldRSSContentTest: #3150661: FileFieldRSSContentTest uses XPath incorrectly

james.williams’s picture

Thanks for picking this up cburschka! That looks good to me now. I guess as we've both worked on the patch, we need someone else to be the one that moves this to RTBC?

Version: 9.0.x-dev » 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.1.x-dev » 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Version: 9.5.x-dev » 10.1.x-dev
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Patch #12 had an error on D10

cburschka’s picture

I've backdated the previous patches into a fork + merge request to work with it more easily.

I suspect the largest problem is the use of file_create_url(), which was replaced with a service in D10 (https://www.drupal.org/node/2940031), and ::drupalPostForm(), which was deprecated in favor of ::drupalGet()/::submitForm() (https://www.drupal.org/node/3168858).

cburschka’s picture

Status: Needs work » Needs review

Fixed the deprecations.

cburschka’s picture

Unfortunately I don't see an easy way to run tests on the testonly branch without opening a second MR.

cburschka’s picture

The test-only patch is failing as designed. This looks ready for review.

smustgrave’s picture

Seems MR 3373 has some failures.

Hiding the files to avoid confusion.

As far as tests-only you are spot on for what I've seen others do. They open a separate MR for the tests-only. If you uploaded a test-only patch and then did an MR after that not entirely sure what happens.

cburschka’s picture

Status: Needs work » Needs review

MR 3373 is supposed to have failures, that's the test-only one :D

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

That's my mistake

Could the issue summary be updated though with steps to reproduce this, the proposed solution, maybe before/after screenshots could help. Helps the reviewer and committer.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.