Amp-wp: Error caused by using is_embed() and is_feed() in is_amp_endpoint() before condition (! $did_parse_query)

[hansschuijff]

Bug Description

When calling is_amp_endpoints() at an early stage in a plugin, the conditionals is_embed() en is_feed() issue an error notice, saying it can't be used before the query is finished. It doesn't reach the code that is used to test if the function is started too early, so the cause is unclear at first.

Expected Behaviour

The function tests if it is started before the 'parse_query' hook and starts doing_it_wrong saying is_amp_endpoint() it is started to early in the process. This would be the intended behaviour when it is started too early. it would be solved by moving the if(! $did_parse_query) block up so it runs and before testing the template tags. now it doesn't reach that test depending on how early it is called.

Steps to reproduce

1. Add a if statement with the condition is_amp_embed() at the global level of a plugin
2. activate the plugin
3. refresh a page at the website's frontend.
4. See error in the log (or whoops)

Screenshots

the notice is:
is_embed was called incorrectly. Conditional query tags do not work before the query is run. Before then, they always return false. Please see Debugging in WordPress for more information. (This message was added in version 3.1.0.)

Additional context

• WordPress version: 5.4
• Plugin version: 1.5.2
• PHP version: 7.3.2

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

• Improve the _doing_it_wrong() message for when is_amp_endpoint() is called before the wp action.
• Account for additional template conditionals: is_comment_feed(), is_trackback(), is_robots(), and is_favicon().
• Remove special condition to allow is_amp_endpoint() to not emit _doing_it_wrong() when called before wp when in Reader mode.
• Prevent emitting _doing_it_wrong() errors when by calling is_embed() or is_feed() before they are able to be called.
• Prevent emitting duplicated warnings.
• Remove separate condition for doing redirects to non-AMP when AMP is not available in Reader mode.
• Remove unnecessary exit logic.
• Fix ability to access the page_for_posts if designated to be enabled in AMP Reader mode.

[westonruter]

Would you like to open a pull request to implement what you are proposing?

[hansschuijff]

I can try. It would be my first time pulling. :)

[westonruter]

Thanks. We'll be happy to help you iterate on the PR.

[hansschuijff]

I'm trying to clone the repository, but get en error and it won't clone (the result is an empty folder). Probably caused by the space in the path (see below). It seems something in the repo.

error: invalid path 'lib/optimizer/tests/spec/transformers/valid/ServerSideRendering/boilerplate_not_removed_when_amp_experiment_present/input.html /input.html'
fatal: unable to checkout working tree
warning: Clone succeeded, but checkout failed.

[westonruter]

Thanks. It looks like you've identified a bug in how we're syncing the test suite down. Namely, in lib/optimizer/bin/sync-amp-toolbox-test-suite.php. We'll investigate and let you know when you can try again.

[westonruter]

@hansschuijff We have a fix in #4527. Could you try cloning that branch?

Assuming you cloned from [email protected]:ampproject/amp-wp.git, you should be able to try that via:

git fetch origin && git checkout update-test-specs

[westonruter]

I went ahead and merged that pull request, so you can just pull down the latest commits from the develop branch (the default) and try checking out again.

[hansschuijff]

Thank you, I am able to clone now.

[westonruter]

@hansschuijff Any update on this?

[hansschuijff]

Hi Weston,
The delay was that I wasn't able to successfully use the scripts in the plug-in. I read in the docs that I should best run start.sh. but since I run my local dev on a windows 10 platform using local from Flywheel and I hadn't yet encountered such scripts, I needed to find out what that was and how to run it. I now know what it is and learned some Linux commands, but still can't get it to work yet since both in ubuntu and in my local flywheel ssh, running start.sh returns errors about the end of line chars and more.

Also running nmp install and npm run dev and then npm run build didn't work for me, and it reported errors I was not capable in solving yet. So it might just be that the dev env for me is yet too complex/advanced. Sorry for that.

It cost me more time than I would have liked without the result I sought. Probably I need to pipe the script through something on a windows platform, but I didn't know how to do that. I guess most of you develop on a unix or Mac platform that don't have this problem. A lot of effort for such a tiny edit.

I have made the change and a pull request and tested the code by just changing it too in the installable version of the plugin. So I think this must be it then. In the code of is_embed() and is_feed() I read that they fire doing it wrong when $wp_query isn't set, so moving those to below like in the pull request results in better error logging.

Hope this helps. Let me know if I need to change something.

[hansschuijff]

By the way: the pull request comment points to documentation links (behind the checkboxes) that no longer exist. Perhaps you want to change that too?

[schlessera]

By the way: the pull request comment points to documentation links (behind the checkboxes) that no longer exist. Perhaps you want to change that too?

Thanks for letting us know, I've created an issue for that: https://github.com/ampproject/amp-wp/issues/4575

[westonruter]

(Re-opening for QA purposes)

[schlessera]

QA passed

The error for calling it too early is fixed: