Repository navigation
Keep the first author instead of the last when a node has several - #141
thejdubb02 wants to merge 4 commits into
Conversation
An Atom entry can carry more than one <author> element (RFC 4287 4.1.2), and RSS/other formats can also repeat <author> or an equivalent per-item author tag. FeedIo\Feed\Node stores a single $author property, and both Author rule classes call setProperty once per matching element with no guard, so each <author> tag unconditionally overwrites the one before it. The result is that whichever author appears LAST in the document wins, which is surprising and loses every author but one regardless of order. Guard both setProperty methods to keep the first author and ignore the rest, matching the parser's own single-author data model instead of silently discarding data based on document order. Reported downstream in nextcloud/news#3898, where readers using this library only ever see the last author on a multi-author entry.
|
So why the first one rather than the last? What makes that more correct? |
|
The first-listed author is the one readers of a single author field generally expect. RFC 4287 does not rank multiple author elements, so document order is the only signal there is, and by byline and citation convention the first-listed author is the primary one. SimplePie does the same: Keeping the last one was never a decision in the code, it fell out of the rule running once per element and overwriting, so the result depended on how many authors a feed happened to list. If you would rather keep all of them I'm happy to do that instead, but it means an authors collection on Node, which is a bigger API change than this fix. I've also added the CHANGELOG entry. |
|
I'd say we should keep all of the entries, keep the last listing but add a getter for Authors |
As discussed in review: getAuthor() keeps returning the last author like before, and every author is now collected and available through getAuthors(). JSON Feed keeps the first entry of authors as getAuthor().
|
Thanks, done. getAuthor() returns the last author again, and every author is now kept and available through getAuthors(). For JSON Feed, getAuthor() still returns the first entry of authors, as before. Tests cover the Atom, RSS and JSON paths. |
| @@ -20,6 +20,8 @@ class Node implements NodeInterface, ElementsAwareInterface, ArrayableInterface | |||
|
|
|||
| protected ?AuthorInterface $author = null; | |||
There was a problem hiding this comment.
I don't think we should keep this, it makes for a very confusing situation. Let's just store the author information only in the array. If that changes the behaviour of getAuthor I think that's okay.
| /** | ||
| * returns all the authors of the node, in document order | ||
| * | ||
| * @return iterable | ||
| */ | ||
| public function getAuthors(): iterable; |
This branch has not been deployed
2 participants
What happens now:
An Atom entry can carry more than one author element, and other feed formats can also repeat an author tag per item. FeedIo\Feed\Node stores a single $author property, and both Author rule classes (FeedIo\Rule\Author and FeedIo\Rule\Atom\Author) call setProperty once per matching XML element with no guard against overwriting. XmlParser::handleNode invokes setProperty once for every child element that matches the rule's node name, so when an item has three author tags, the rule runs three times and each call unconditionally replaces the previous author. Whichever author appears last in the document is the one that survives.
Root cause:
Neither Author rule class checks whether the node already has an author before building a new one and calling setAuthor. The fix adds that check to both classes: if the node already has an author, the rule returns without doing anything, so the first author encountered is the one that is kept.
Steps to reproduce:
Parse an Atom entry with two author elements, for example name John Doe followed by name Jane Roe. Before this change, item.getAuthor().getName() returns Jane Roe. After this change, it returns John Doe. The same applies to the generic Author rule used by other formats.
Verification:
Added a regression test to both AuthorTest classes that calls setProperty twice with two different authors and asserts the first one is kept. Both new tests fail against the current code (they return the second author) and pass with the fix. Ran the full suite with make test: 299 tests, 761 assertions, all passing. Ran vendor/bin/phpstan analyse at the project's configured level: no errors.
This is reported downstream as nextcloud/news#3898, where a Nextcloud News reader only ever sees the last author on a multi-author entry. I did not touch anything outside the two Author rule classes and their tests.