search

GoogleChromeLabs / wp-sitemaps

Proposal to integrate basic XML Sitemaps in WordPress Core
GNU General Public License v2.0
97 stars 26 forks source link

Road towards WordPress Core #164

Closed swissspidy closed 4 years ago

swissspidy commented 4 years ago

This thread aims to cover things needed to pave the way for an eventual merge proposal into WordPress core.

Sub-issues will be created as-needed.

The tasks are more or less in desired chronological order:

adamsilverstein commented 4 years ago

@swissspidy I will take a pass at "Update all inline documentation to adhere to WordPress coding standards" - is there anything in particular you noticed that needs adjusting? I see that phpcs is in place and enforcing having inline docs :).

I do see that the language for descriptions is a bit off - the documentation standards call out using third-person singular verbs which we do not - I will start with that.

felixarntz commented 4 years ago

@adamsilverstein The 3P singular verbs was I was thinking about too here. We should also consider updating the since annotations - many are missing, and the ones are there refer to plugin versions. Maybe we should already set these to 5.5 instead, or something that's easy to replace with the exact WP version later?

adamsilverstein commented 4 years ago

@felixarntz Good idea, I'll set them all to @since 5.5.0 since that is our target for merge. I spotted a few other irregularities along the way, mostly though, this code is very well documented!

felixarntz commented 4 years ago

@swissspidy I looked through the codebase a bit, opened a few small PRs (#170, #171, #172), and will now have a look at #124.

adamsilverstein commented 4 years ago

@swissspidy I started work on documentation standards updates in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/173 - let me know if there is anything else you know is missing and I'll finish that up.

adamsilverstein commented 4 years ago

I will take this one: "Get audit from WordPress security team"

adamsilverstein commented 4 years ago

I will work on these items from the Merge criteria:

I don't think these items apply, since the plugin doesn't include a UI (unless I missed it):

pbiron commented 4 years ago

I don't think these items apply, since the plugin doesn't include a UI (unless I missed it):

  • [ ] The user interface/experience has been tested through user testing, and appropriate feedback was incorporated.
  • [ ] Any required accessibility support has been added, including (but not limited to) off-screen text, ARIA, and JS-assisted.

While there's nothing like a Settings UI, there is the HTML rendering of the sitemaps (and sitemap index), e.g. https://example.com/wp-sitemap.xml). Should that be checked for a11y?

swissspidy commented 4 years ago

Should that be checked for a11y?

Not sure how much we can do about a11y there, but yeah makes sense.

pbiron commented 4 years ago

Maybe there are no a11y issues with what the stylesheets output. But, not being an "a11y guy", maybe we need to add/fix some things? At any rate, I think it deserves a review.

felixarntz commented 4 years ago

@swissspidy Happy to tackle the part of using more core-like names for all the classes and functions in preparation for a patch. I can do that end of this week.

swissspidy commented 4 years ago

@felixarntz sounds good! Should we merge the open tests PR before that to avoid unnecessary conflicts?

felixarntz commented 4 years ago

@swissspidy Yeah, that'd be great! From my end that's good to go.

swissspidy commented 4 years ago

@felixarntz Merged! For the refactor, keeping #91 in mind would be good (making names consistent)

All:

131 would be a nice little thing to solve too, adding a filter for the arguments passed to WP_Query and WP_User_Query everywhere. Plus pre_* filters to short-circuit those queries.

134 could be handled in core, IMHO. WDYT?

151: I don't think we need a full-fledged API for extension. If devs can easily add lastmod, changefreq, and priority and that those also show up in the stylesheet (e.g. by using a second filter), it would be good enough IMO. A more robust API for namespaces could be deferred to until post-merge. Thoughts?

adamsilverstein commented 4 years ago

I requested a docs review from the core Docs team and will follow up with them at their next meeting (https://wordpress.slack.com/archives/C02RP4WU5/p1588858440176300)

adamsilverstein commented 4 years ago

Not sure how much we can do about a11y there, but yeah makes sense.

Can't hurt to ask!

adamsilverstein commented 4 years ago

I requested a review from the accessibility team and will follow up with them at their next meeting (https://wordpress.slack.com/archives/C02RP4X03/p1588858740465100)

adamsilverstein commented 4 years ago

I requested a review from the Polygots team and will follow up with them at their next meeting (https://wordpress.slack.com/archives/C02RP50LK/p1588859081157000).

adamsilverstein commented 4 years ago

I will take this item: "Create ticket on WordPress Trac"

adamsilverstein commented 4 years ago

Created a trac ticket: https://core.trac.wordpress.org/ticket/50117

zzap commented 4 years ago

I requested a docs review from the core Docs team and will follow up with them at their next meeting (https://wordpress.slack.com/archives/C02RP4WU5/p1588858440176300)

I reviewed everything except tests. Here's the list of issues I found.

Default value

Every string @param with empty value was documented with Default ''. It should be Default empty.

@return description

On two places these descriptions started with small case. It should be upper case.

Array @param

For params that are array of arguments, array values should be documented using WordPress’ flavor of hash notation style (WordPress Coding Standards):

/**
 * @param array $args {
 *     Optional. An array of arguments.
 *
 *     @type type $key Description. Default 'value'. Accepts 'value', 'value'.
 *                     (aligned with Description, if wraps to a new line)
 *     @type type $key Description.
 * }
 */

Inconsistencies

Empty line between @param and @return is not consistent. Mostly there's no empty line but it is found on few places.

The term sub type appears in 3 different forms:

Other

Typo in https://github.com/GoogleChromeLabs/wp-sitemaps/blob/master/inc/functions.php#L18

Core_Sitemaps instance, or null of sitemaps are disabled.

I guess it should be if.

docs/README.md

Reporting Security Issues link leads to 404.

https://github.com/GoogleChromeLabs/wp-sitemaps/blob/master/docs/README.md#reporting-security-issues

docs/TESTING.md

Typo:

If you your site is not running

https://github.com/GoogleChromeLabs/wp-sitemaps/blob/master/docs/TESTING.md

docs/CONTRIBUTING.md

Create a new branch, forked from develop.

There is no develop branch in this repository.

https://github.com/GoogleChromeLabs/wp-sitemaps/blob/master/docs/CONTRIBUTING.md#developer-contributions

This is it. Hope it helps.

swissspidy commented 4 years ago

@adamsilverstein Thanks for creating the ticket! Probably warrants a new Sitemaps component on Trac after merge :-)

@zzap Really useful, thanks!

adamsilverstein commented 4 years ago

@swissspidy Discussing review with the Polygots team, they asked about whether we have a plugin in the .org repository. Have you considered this already? It would enable wider testing and is listed as a merge requirement. Might be worth discussing at the next meeting.

adamsilverstein commented 4 years ago

@zzap Thank you so much for the detailed review. I will work to address these issues.

Empty line between @param and @return is not consistent. Mostly there's no empty line but it is found on few places.

Reviewing the documentation guide, the examples do not have a blank line before the @return line. I will follow that approach, if that is not right please let me know!

zzap commented 4 years ago

@adamsilverstein That is correct. Current rules do not enforce blank line before @return and the practice in core is to omit it. You can follow that approach.

swissspidy commented 4 years ago

@adamsilverstein the plugin is already in the repo: https://wordpress.org/plugins/core-sitemaps/

adamsilverstein commented 4 years ago

@swissspidy Doh! Thank you for sending the link.

adamsilverstein commented 4 years ago

@zzap Thanks again for the thorough review.

I have tried to address all the items you noted in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175. The only thing I felt a little unsure about was the array notation, so if you have a chance I would appreciate you checking to make sure I did those correctly.

Default value Every string @param with empty value was documented with Default ''. It should be Default empty.

Addressed in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/ef2993c65d2416d202fc3a5df583d5de19fc13b7

@return description On two places these descriptions started with small case. It should be upper case.

Fixed these as well as a few @return items that were missing descriptions in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/77ee92a33628a6857fb51c5d27fb1f90698019b0

Array @param For params that are array of arguments, array values should be documented using > WordPress’ flavor of hash notation style (WordPress Coding Standards):

Attempted this in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/c25bbf30d5e34ebfe6bab554d96c4762c0003c55

Might need another review to make sure I got this right.

Inconsistencies Empty line between @param and @return is not consistent. Mostly there's no empty line but it is found on few places.

Removed empty lines before @return statements in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/feb21c2895a638601efde42b4f55e76557ed22d4

I found this only in one place, in others the @return statement followed the description or @since and there I left a space.

The term sub type appears in 3 different forms.

I settled on subtype (which seems to be the standard spelling) in

Fixed in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/3d3271d1aef0cb8895ccb5b3d394ea1d42260936 (oops and https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/721766540ee7dff432ea6f37bdd797a3ce61a336)

Other Typo in /inc/functions.php@master#L18

https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/032ab5784068d78fb660269fbf4fd5d011ecf4e9

docs/README.md Reporting Security Issues link leads to 404.

Fixed in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/c1a08fa76ff88dd00b0f5343eab0e3de7a38ac00

docs/TESTING.md Typo:

Corrected in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/ee2d6ca500ee9dc556a3bbbdaa24fcab06bd0f4a

docs/CONTRIBUTING.md Create a new branch, forked from develop. There is no develop branch in this repository.

Changed references to master in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/fdcc2de0a5089e386755b5ec47ee0984378a79bd

zzap commented 4 years ago

@adamsilverstein:

I have tried to address all the items you noted in #175. The only thing I felt a little unsure about was the array notation, so if you have a chance I would appreciate you checking to make sure I did those correctly.

You didn't have to modify all arrays. I'm very sorry that my explanation was unclear. The only candidate for documenting array @param in this way is the one I linked and you did it almost perfect.

The only thing that should be changed here is that arguments are not strings but objects. So it should be:

/**
 * @param array $providers {
 *     Array of Core_Sitemap_Provider objects.
 *
 *     @type object $posts      The Core_Sitemaps_Posts object.
 *     @type object $taxonomies The Core_Sitemaps_Taxonomies object.
 *     @type object $users      The Core_Sitemaps_Users object.
 * }
 */

Now, let me explain why is this needed here and not on other places. The array $providers is an associative array. Its keys are arguments and not something we can know unless we look at the source code. It's not just array of post IDs or post objects. Those arrays are indexed and it's quite clear what I have to provide.

But here I don't know how are keys named nor how many of them are there. So when I want to filter this array I don't know which values I can change.

The great example of this in core is WP_Query::parse_query(). These are all arguments we can set for a query. In order to make the query successful we need to know all available arguments because we can't just guess their names and expected type.

Another interesting example is get_posts() which uses all the arguments from WP_Query::parse_query() but doesn't document them all. Instead, it refers to see WP_Query::parse_query() for all available arguments.

For the rest of arrays in this plugin, I suggest a slight rewording to be more clear. For example, this is not clear:

@param array $sitemaps List of sitemap entries.

In WordPress ecosystem, entries more refer to objects (e.g. post type objects). In PR I see these are sitemaps' URLs. If each sitemap is an object then its URL could be called permalink? I'm guessing here but the idea is to describe that $sitemaps is an array of sitemaps' URLs.

Also, I overlooked last night the wording for arrays. It should be Array in description, not a List. This should be changed for all occurrences.

So the next one is quite clear. It is an array of URLs for the sitemap. Just swap list for array and it's good to go.

@param array $url_list A list of URLs for a sitemap.

The next one could use a bit of clarification but still doesn't need to be documented in array format. 

@param array $post_types List of registered object sub types.

I would keep the subtype term here but would also explain what it is. Subtype is not something we are used to see in WordPress so I would describe it like this:

@param array $post_types Array of registered subtype objects (WP_Post_Type).

The same goes for taxonomy objects. taxonomy type name sounds more like a string. I would make it clear it's an object.

@param array $taxonomy_types Array of registered taxonomy type objects (WP_Taxonomy).

And the last one I'd word like this:

@param array $url_list Array of URLs from a sitemap provider.

Again, I'm sorry for not being more clear. Hope this is a better explanation. If you have any questions do let me know.

adamsilverstein commented 4 years ago

@zzap thanks for the clarifications and especially explaining why. I’ll address these in my PR.

adamsilverstein commented 4 years ago

You didn't have to modify all arrays. I'm very sorry that my explanation was unclear.

@zzap - Oops!

I reverted that commit (yea, granular commits) and replaced the one instance that needed documenting that way, including your suggested change to object. Then I went thru and cleaned up all the descriptions.

Fixed everything up in https://github.com/GoogleChromeLabs/wp-sitemaps/pull/175/commits/2ea150c2963339febd6144f10aa49045e5c342d0 ready for another review please!

zzap commented 4 years ago

Looking good @adamsilverstein :+1:

adamsilverstein commented 4 years ago

Drop core_sitemaps prefix in functions, hooks, and class file names.

I'll take this one.

swissspidy commented 4 years ago

Started a pull request here: https://github.com/WordPress/wordpress-develop/pull/310