Skip to content
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

Some page URL lookups behaving strangely #14

Closed
notabeatle opened this issue Aug 31, 2020 · 12 comments
Closed

Some page URL lookups behaving strangely #14

notabeatle opened this issue Aug 31, 2020 · 12 comments

Comments

@notabeatle
Copy link

Version: 2.9.0

Situation:
Site uses a page as its homepage (the URL of which is typically retrievable with get_option('page_on_front')). This happens to be named "desktop", in this case. Without nLingual, and under nLingual 2.8.10, this resolves simply to example.com/ with no page slug. Under nLingual, the home page is redirected to example.com/desktop/, and get_option('page_on_front') not only returns that desktop/ path while on the homepage, but proceeds to return whatever the current page is, as one nagivates around the site, such that a link that would normally always point at the homepage (simply, example.com/) points instead to, say, example.com/some-page while visiting some-page.

So there seem to be two things going on, that are different from behavior under vanilla WP or nLingual 2.8.10:

  1. A page that normally serves as the front-page, is now explicitly redirected to when one visits the home page (load example.com, end up on example.com/desktop)
  2. WordPress is unable to locate the home page URL via the page_on_front option, and ends up providing the current page path for some reason.

The first is just undesirable, but the second actually breaks things.

Possibly relatedly, I'm also seeing page URL lookups built into WooCommerce, for things like the cart or checkout URLs, return a link to the site root instead, with the result that these links are non-functional with nLingual 2.9.0 enabled. There seems generally to be something odd happening with internal WordPress page URL lookups, though not with lookups for the purposes of request routing, as far as I can tell (if you put the URLs in manually, they work).

(It may be premature to report bugs in 2.9.0 since it's not up on the plugin market yet, but given the version bump on master I thought it safe to at least try out; if you already knew or suspected it has issues of this sort and aren't ready for anyone to be messing with it, I'll take no offense at a fast close on this issue)

@dougwollison
Copy link
Owner

That's strange, I had noticed this issue yesterday while testing it on a few dev sites, but it should be resolved. I'm so far unable to recreate the issue, even with a fresh install using nLingual + WooCommerce + TwentyTwenty. So far the home URL isn't redirected to the page's normal name, nor does the home link break on other pages.

Can you give me an outline of what setup you're using?

@notabeatle
Copy link
Author

notabeatle commented Aug 31, 2020

Haha, we have a pretty extreme plugin count and a lot of custom code, so it could well be some bad interaction there. This time I didn't reproduce with a fresh installation like I did the last time I bugged you, so it may just be our environment. I did confirm that it doesn't happen without nLingual enabled, and that it doesn't happen with 2.8.10. We are still on 5.4, not 5.5, [edit: of Wordpress, that is] if that matters or is known to cause problems, though I bumped to latest WooCommerce on our testing instance to see if that was the culprit (it wasn't).

If I get a chance today I'll try to reproduce the issue on a fresh, blank 5.4 installation, and if the behavior crops up there I'll try going to 5.5 and see if that resolves it. If it doesn't show up in either case then I've got some digging to do, I guess. Thanks for checking it out.

@dougwollison
Copy link
Owner

I don't think 5.5 has anything to do with it, though I'm wondering if it has something to do with the change I made to Rewriter::localize_here() where it first checks if it's the homepage, then checks if it's some kind of post/page/etc.

Regarding issue 2, it sounds like the link is in fact printing an empty string; can you confirm that the actual markup is printing the current page link.

My next guess is that something is causing page_on_front to return 0/null and so get_permalink is simply using the current post's URL. I'll do some digging to see what could've changed that affects that.

@notabeatle
Copy link
Author

OK, here's what I just tried:

  • Fresh wp 5.4 (since it what my testing docker file was set to from last time and 5.5 shouldn't make a difference)
  • nLingual 2.9.0 from git, added via mapped directory, so the only plugins I see after initial site set-up are that and Akismet (both disabled initially)
  • Add a page called "test homepage" with a header reading the same.
  • Set my home page to a static page, and to "test homepage", which happens to have an id of 5.
  • Note: my permalink settings are at WP default
  • Visit homepage. Shows my static page, URL does not change from /
  • Enable nLingual. Do nothing else. No changing settings, no adding languages.
  • Visit homepage. URL now shows ?page_id=5&nl_language=en

These show up as 302 redirects (from /) in my logs.

Then I tried setting URLs for the default language will be unmodified., with the result that the redirect dropped the nl_language part but retained the rest (/?page_id=5)

Then, enabled proper permalinks in WP settings. Redirect still happens, but now to /test-homepage/, as seen on our site.

Disable nLingual, behavior reverts to default (no redirect).

Now here's something odd:

  • nLingual deactivated
  • Visit homepage
  • Observe that there are two entries in the default "pages" menu at the top, "Sample Page" and "Test Homepage"
  • Observe that these link to /sample-page and / respectively.
  • Enable nLingual
  • /sample-page is unchanged, but Test Homepage now links to /test-homepage

This is all under theme Twenty Twenty (the default).

So it does seem to be happening with vanilla WP, unless I've done something wrong here.

HOWEVER I was not able to find a way to reproduce links intending to point at the homepage linking instead to the current page. I haven't dug far enough into Twenty Twenty's header code to tell how they're generating those links, yet.

@dougwollison
Copy link
Owner

Okay that's just maddeningly strange. I'll do somet further testing tonight to see if I can recreate it.

@notabeatle
Copy link
Author

Just tried WP 5.5, same behavior. Under PHP 7.2 in both cases. Just bumped up to 7.4.9 just to cover all my bases. Same behavior.

@notabeatle
Copy link
Author

version: '3.1'
# Default mysql user: root
services:

  db:
    image: mysql
    command: --default-authentication-plugin=mysql_native_password
    restart: always
    environment:
      MYSQL_ROOT_PASSWORD: nlingual
    ports:
      - 3306:3306

  adminer:
    image: adminer
    restart: always
    ports:
      - 8091:8080

  wordpress:
    image: wordpress:5.5.0-php7.4-apache
    depends_on:
      - db
    restart: always
    environment:
      WORDPRESS_DB_USER: root
      WORDPRESS_DB_PASSWORD: nlingual
      WORDPRESS_DB_HOST: db
      WORDPRESS_DEBUG: 1
    volumes:
      - ./plugins:/var/www/html/wp-content/plugins:rw,z
    ports:
      - 8090:80

Quick & dirty docker-compose.yml I've been using to test, in case it's useful. You have to go through WP setup on first run, but as long as you don't rm the database container it shouldn't make you do that again. Just dropping nlingual in that mapped ./plugins directory straight from git, as in ./plugins/nlingual, so there's no need to install it after WP is up, should already be there if you put the plugin in place ahead of time.

The :rw,z permissions mods on the end of that directory mapping may not be necessary on macOS, but were on Linux. No clue how to make it work if you're on Windows, as I've never used Docker there.

WordPress is served on port 8090 with this file, unless you change it.

@dougwollison
Copy link
Owner

Okay, got a docker instance spun up (holy crap I need to start using this more), and was able to recreate the issue.

Digging into the cause now but as far as the nl_language=en bit that's technically intended, due to the default permalink "structure" being via GET params, and the default handling being to localize URLs for the default language. It's not an outright issue but I'm debating changing some of the defaults in the next release.

@notabeatle
Copy link
Author

Right, I get that nl_language=en is working as intended, and that part's fine. Just included observations about its behavior under different settings for completeness. The way it dropped off the redirect to page #5 with default permalink structure maps just fine to how it works with page-slug redirects, I think, and it was (correctly) absent from default lang permalinks when default language redirects were disabled.

Let me know if I can do anything else to help out. Thanks a bunch for looking at this.

@dougwollison
Copy link
Owner

Okay I found the culprit:

$translation_id = Translator::get_post_translation( $post_id, $current_language );

When I rewrote the current_language_post hook to check and make sure it wasn't returning an unpublished post, I didn't include logic to handle get_post_translation returning false (for un-assigned/translated posts). This caused is_front_page() to return false because it's comparing page_on_front (which was rewritten to false/0) to the current page. Technically this didn't break anything until WP_Query finishes setup and sets the $post global, which means any later attempts to rewrite page_on_front cause it to compare the queried post (the real homepage) to itself (the default post when passing null/false/0 to a function like get_post_status) rather than a non-existent one.

It's fixed now, as for the other issue with the homepage links pointing to the current page, not sure what could be happening there but might be tangentally related to page_on_front getting set to 0.

@notabeatle
Copy link
Author

Just tried out master on our testing site. All issues appear to be resolved, including the weirdly-broken home links.

Quickly kicked the tires on client language preference redirection and that still seems to be good, no regression.

Awesome. Thanks again.

@dougwollison
Copy link
Owner

Sweet. Thanks for all the detailed info, really appreciate it.

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

No branches or pull requests

2 participants