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.
Comment | File | Size | Author |
---|
Issue fork drupal-3150318
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:
Comments
Comment #2
james.williams CreditAttribution: james.williams at ComputerMinds commentedHere's an attempt, with a test that is based on the one for regular file fields (
FileFieldRSSContentTest
).Comment #3
james.williams CreditAttribution: james.williams at ComputerMinds commentedI 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).
Comment #5
james.williams CreditAttribution: james.williams at ComputerMinds commentedPerfect, that was as expected - the test-only patch failed, but the fix+test passed.
Comment #6
cburschkaPatch 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.
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.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
or
We also have a BrowserTestBase::xpath() function which takes over the sprintf functionality, so the whole thing could be shortened to
But that last one's subjective.
Comment #7
james.williams CreditAttribution: james.williams at ComputerMinds commentedThanks 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.
Comment #9
cburschkaApparently ::xpath()/::buildXPathQuery() automatically encloses arguments in double-quotes.
I've also simplified this part:
since it looks like $uploaded_filename isn't used anywhere else
Comment #11
cburschkaI'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.
Comment #12
cburschkaOops - 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.
Comment #14
cburschkaI filed a separate issue for the fix to FileFieldRSSContentTest: #3150661: FileFieldRSSContentTest uses XPath incorrectly
Comment #15
james.williams CreditAttribution: james.williams at ComputerMinds commentedThanks 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?
Comment #20
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis 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
Comment #22
cburschkaI'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).
Comment #23
cburschkaFixed the deprecations.
Comment #25
cburschkaUnfortunately I don't see an easy way to run tests on the testonly branch without opening a second MR.
Comment #26
cburschkaThe test-only patch is failing as designed. This looks ready for review.
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems 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.
Comment #28
cburschkaMR 3373 is supposed to have failures, that's the test-only one :D
Comment #29
smustgrave CreditAttribution: smustgrave at Mobomo commentedThat'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.