Skip to content

Update cat base to handle redirects properly #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chuckreynolds
Copy link

Updated to handle redirects properly.
It was doing /blog/category/foocat/ => /foocat and 404'ing... This patch should handle all/most? (test) redirects accordingly; example /blog/category/foocat/ => /blog/foocat/

Updated to handle redirects properly. 
It was doing /blog/category/foocat/ => /foocat and 404'ing... 
This patch should handle all/most? (test) redirects accordingly; example /blog/category/foocat/ => /blog/foocat/
);
// Determine blog prefix, similar to modify_category_rewrite_rules
$permalink_structure = \get_option( 'permalink_structure' );
$blog_prefix = \is_main_site() && str_starts_with( $permalink_structure, '/blog/' )
Copy link
Owner

@sybrew sybrew Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Question: Why are we testing is_main_site() here?

Perhaps we should use home_url( '/' ) and let WordPress figure out the base, instead of using get_option( 'home' ) (at line 111).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is_main_site()... idk. kinda vibe coded it while drinking on vacation :) (only time i have to work on non-work stuff) Probably don't need that. GPT4.1 probably saw

$blog_prefix = \is_main_site() && str_starts_with( $permalink_structure, '/blog/' )
in modify_category_rewrite_rules() and duped that functionality.
It also feels a little 'hard coded' to the use case of .tld/blog/**

Use get_category_link() to get the correct URL for the category by slug.
Use the slug from the redirect match to get the term object.
Redirect to the canonical category URL using get_category_link()
@chuckreynolds
Copy link
Author

chuckreynolds commented Jun 11, 2025

New way to handle it. Getting cat url/slug instead as it already is handled correctly so may as well use that instead of recalcing what the slug should be. @sybrew

Edit 1: That said I still think this in modify_category_rewrite_rules() is hard-fixed to /blog/ being in the permalink structure.

$blog_prefix = \is_main_site() && str_starts_with( $permalink_structure, '/blog/' )
    ? 'blog/'
    : '';

That'd need to be tackled if this were to be released more publicly.

Edit 2: Looking at YxxSEO plugin that's what they have; so is_multisite came from that too in /inc/class-rewrite.php

$permalink_structure = get_option( 'permalink_structure' );

$blog_prefix = '';
if ( strpos( $permalink_structure, '/blog/' ) === 0 ) {
	if ( ( is_multisite() && ! is_subdomain_install() ) || is_main_site() || is_main_network() ) {
		$blog_prefix = 'blog/';
	}
}

}

// Fallback: redirect to home if not found
wp_safe_redirect( home_url(), 301 );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never do this! 404s are good. I smell a bit too much vibe 😅

/**
* Helper: Get category by slug.
*/
function get_category_by_slug( $slug ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function already exists in Core: https://developer.wordpress.org/reference/functions/get_category_by_slug/. It's heavy, though. If possible, I'd rather trim out the slug outright instead of performing category lookups.
In other words, we probably just want to modify the rewrite base so that /%category%/ disappears.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have at it... you're a better coder than me most likely... i'm out of the game mostly lol. Too much dust for me to knock off to code full-time anymore. I just want a solution that's solid and can be shared back to the community for everybody to use.

@sybrew sybrew requested a review from Copilot June 20, 2025 20:13
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the category base removal plugin to handle category redirects more accurately by using the category's slug to fetch the proper link.

  • Updated plugin version from 1.0.0 to 1.0.2
  • Revised redirect logic in redirect_base() to use get_category_by_slug() for fetching the category link and added a fallback redirect
  • Introduced a helper function get_category_by_slug()
Comments suppressed due to low confidence (1)

permalinks/tsf-remove-category-base.php:120

  • Consider adding a detailed docblock for get_category_by_slug, documenting the expected type of the parameter and the return value to clarify its usage.
function get_category_by_slug( $slug ) {

@sybrew
Copy link
Owner

sybrew commented Jun 20, 2025

Well, that was a waste of $390 🥲, I thought Copilot would consider our conversation and amend the commit accordingly.

@sybrew
Copy link
Owner

sybrew commented Jun 20, 2025

I'll look at this again later (I'm extremely swamped) -- if I were confident that this feature would be foolproof, it'd have been implemented in TSF a long time ago.

Due to performance issues I mentioned in the review, I'm skeptical about the PR as it stands, even though the suggestion you're making could make it much more robust.

I'll keep this PR open for now, because it does solve a problem, though I need to get my head into it to figure out if it's the best and the only solution. Tweaking is necessary, such as removing the fallback redirect, but that can always happen later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants