Open bwmarkle opened 4 years ago
after reading over @jessecowens changes and doing some research on the canonicals on archives - the changes make sense, and should result in posts that might have been buried deeper (as might be the case with old support center articles) to surface in searches ie things that might be in pages 2, 3, 4, etc of pagination should have better search visibility. After reviewing the code changes though - they work in the scenarios I tested, but digging into wordpress-seo's implementation, which from what I gathered (untested) achieves this same goal for paginated lists of pages, seems much more elegant. I was curious if you had taken a look at how they are generating the canonicals in: https://github.com/Yoast/wordpress-seo/blob/f38c351d11026f9669b45ce394173f1d21ee1ff6/frontend/class-frontend.php#L909
My opinion is that we know Yoast does SEO well, and usually makes the best decisions - so I think what you have so far is a good step in the right direction, and a good thing that you have pointed out for this plugin to fix that aligns with that plugin's philosophy. I do think having the adjacent links is important to include for the archived pages if we go this route though. In reviewing wordpress-seo's code it appears they have also taken a few additional precautions to ensure the detection of the paginated pages is actually paginated - and a few other things that appear we might have failed in this check initially.
I think this would be a really good feature to iron out to make the seo plugin more useful/better, but I do think that the PR needs to have someone sit down through all the scenarios like term queries, search queries, etc to ensure that we have everything solid. The current implementation isn't the best practice to follow, but was the easiest to implement quickly and isn't a negative impact (fixing/changing it to this would definitely be a big positive impact for sites with a decent amount of content though).
My other suggestion before merging this might be to actually test this out in your realm @jessecowens - see what impact the changes you made have on some of the buried posts from support center as a lot of articles have accumulated on bg.com. It's a good change - and the right change, but this is probably most relevant to your needs over the most common users of this plugin atm. The canonical setup right now for the seo plugin is really barebones and meant to provide the base setup for users without them having to learn or do too much additional work - and making the change would definitely benefit the automation of that further and enhance what was already in place. I'd just really like to see more thorough testing on boldgrid-seo side (perhaps unit tests with dummy data could be a possibility to ensure we catch the situations we need) before we merge these changes in. I only say this as the consequences of setting wrong canonicals would be a negative impact, and could even lead to penalties if things got cross referenced incorrectly or bad links exposed just from overlooking something simple.
Another thing I am unsure of is if the front page of a site is set to display a blog (archive possibly paginated), then I think an additional check might need to be added for that scenario so the canonical uses $GLOBALS['wp_rewrite']->pagination_base
also.
As initially we were going to work on a partnership with AllInOneSEO the prexisting code has been mostly untouched, and mainly used just to fall in line with that plugin and keep structures somewhat the same to make upgrades or import/exporting between this and that an easier process. As that hasn't been any sort of focus for a long time now, I think it would be safe to ditch most of this original code added for generating the canonicals and use a more streamlined approach that has proven a good track record. I think the best route at this point would be to dig deeper into wordpress-seo's implementation as it is done better, has more extensive checks, and should be relatively easy to port the functionality over as the goals for each of these methods are pretty much identical - create sensible canonicals automatically on the frontend, and provide an override field per page/post for more advanced users. I'd rather use wp-seo's implementation as it would address this issue and @jessecowens' PR directly by doing exactly what is desired from what is outlined in #14
This also leads into one of the existing issues within boldgrid-seo as the level of control per page/per post is decent, but for instance setting a custom canonical for a category or custom post type is not a feature available for a normal user. Competitor plugins do offer the same level of control for "dynamic" pages created in WordPress, but is something missing from boldgrid-seo as the initial goal was to make a lightweight/no fuss seo plugin for page/post content.
As one last minor discrepancy as I tested this vs yoast that would be a good additional is to not print canonicals if something is set to noindex. This was something I noticed they were doing, and made it easier for me to troubleshoot and flip back and forth between the site, the two plugins, and search console to compare what was happening collectively. I am unsure if this has any negative impact since it's noindexed anyways, so the canonical should be read/crawled from the noindexed page in the first place. If anything the page was a tad faster without the overhead of generating the canonical based on the current query since it was just checking if it was noindexed and skipping over creating a canonical at that point. Not a major issue, but relevant to this change/discussion.
Please see https://github.com/BoldGrid/boldgrid-seo/pull/15