lastBuildDate in comment feed may not be empty

[Cybr]

Hi,

Comment feeds have added the <lastBuildDate> tag, which may not be empty.

However, when no comments have been assigned, this field will be empty. Rendering an invalid feed, according to the W3C.

I hope this helps! :)

[stevenkword]

This may have bee introduced in https://core.trac.wordpress.org/changeset/32765.

I'm looking into it.

[stevenkword]

@Cybr It looks to me that the function should never return an empty string -- even when there are no comments in the system. When there are no comments, it should fallback to the post publish date.

Can you please provide steps to reproduce?

[Cybr]

Hi @stevenkword,

Thanks for looking into this.

It's quite complex, and if you feel like it, please read on. Otherwise, go to the tl;dr section below.

It shouldn't return empty, but it will:
This is because the $wp_query->is_comment_feed() is true on the comment feed, so it will return early with a max() statement, which then will return null when no comments are available in the posts.

When I take a look at the function get_last_build_date_feed(), I can see it can make max() return false on two occasions.
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/feed.php?rev=33224 (line 111 and 116).

When listening to these max() rules:

1. When comparing two different types, the first one found will be used (which can be null).
2. Comparing an empty Array will always return false.

And since max() is used twice, it has the possibility to return false twice:

1. Having no posts will result in an empty array (and thus false).
2. Having no comments or posts will result in null on the SQL query, which is then used as the type setter.

mysqltodate() will return false if the 2nd parameter is empty. And when converting this to a PHP string on echo: ''.

tl;dr:
Taking all these variables into account, I believe this bug is achieved by either removing all approved comments, or having no posts available.

Removing all the approved comments is the easiest way to test this bug.

Having the lastBuildDate output being put into a variable and checking if it's there before outputting that line will solve this issue.
Please be aware that if ( ! empty( $var ) ) is three times as slow as just if ( $var ), since we're dealing with scalar types (which we are).

I hope this clears things up!

[stevenkword]

@Cybr Thanks for the extensive feedback :)

Which feed is rendering as invalid the main comments feed or one of the post comment feeds?

[Cybr]

Hi @stevenkword,

Anytime :)!

Both, actually:
http://example.com/comments/feed/
http://example.com/post-title/feed/

[stevenkword]

At first I thought this related to https://core.trac.wordpress.org/ticket/4575 via changeset [32765]. However, since that was later reverted in [33281], it looks like the problem existed prior to those changes.

Currently in trunk, I am able to move all comments to the trash and view domain.com/comments/feed/ and get the empty <lastBuildDate> tags you mention. In this case, it is more likely that the empty string is being caused by an empty return value from get_lastcommentmodified()

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/feed-rss2-comments.php#L46

[Cybr]

That's correct @stevenkword,

My statements were indeed for get_last_build_date_feed(), a function that never really existed. You spoke about this function in the linked ticket (4575). I back-traced a never-released changeset, and I now know I need to be more careful on this.

I think enough information has been supplied on this matter :).

[stevenkword]

Steps to reproduce:

1. Enable pretty permalinks
2. Move all comments to the trash
3. Visit domain.com/comments/feed/

I have confirmed that this is a valid bug report, but am a little bit on the fence as to how to address the problem. A simple solution would be to have the date fall back to the last published posts, preventing an empty string from ever appearing in the feed. However, I'm worried that by doing so, we would be introducing unexpected behavior out of get_lastcommentmodified().

I'd like to revisit #4575 since it would address this issue via the introduction of a new method, get_last_build_date.

[Cybr]

Why not something more simple and right to the point?

e.g.:

<?php if ( $last_build_date = get_lastcommentmodified('GMT') ) : ?>
        <lastBuildDate><?php echo mysql2date('r', $last_build_date); ?></lastBuildDate>
<?php endif; ?>

I'm unsure about the maintainability, though.

[stevenkword]

Replying to Cybr:

Why not something more simple and right to the point?

e.g.:

<?php if ( $last_build_date = get_lastcommentmodified('GMT') ) : ?>
        <lastBuildDate><?php echo mysql2date('r', $last_build_date); ?></lastBuildDate>
<?php endif; ?>

I'm unsure about the maintainability, though.

I'm not a huge fan of introducing new logic at the template level. Test coverage for Feeds is already minimal and doing so would make future testing more difficult. In addition, I feel like we should always render the tag. The feed is always "built" and should therefore always have that tag. I see that it's optional according to spec, but in this case, I think we need to render it and for it to be accurate. This is why I want to revisit #4575 -- it not only ensures this lastBuildDate wouldn't be empty, but it would ensure it was correct.

In other words -- rather than just specifically put a band-aid on this problem -- we should fix the feed behavior.

[stevenkword]

I've refreshed the patch: https://core.trac.wordpress.org/attachment/ticket/4575/4575_7.diff which should resolve the reported issue here.

[stevenkword]

This was fixed in changeset 38925

[swissspidy]

Duplicate of #38027.