Skip to content

Fix AMP integration by deferring masterbar setup to wp action#11088

Merged
jeherve merged 2 commits intoAutomattic:masterfrom
westonruter:fix/amp-masterbar-integration
Jan 8, 2019
Merged

Fix AMP integration by deferring masterbar setup to wp action#11088
jeherve merged 2 commits intoAutomattic:masterfrom
westonruter:fix/amp-masterbar-integration

Conversation

@westonruter
Copy link
Copy Markdown
Contributor

@westonruter westonruter commented Jan 4, 2019 •
edited by jeherve
Loading

Fixes #11082
Fixes #11081

Changes proposed in this Pull Request:

  • Defer masterbar setup until the wp action, instead of doing the setup at the init action. This is needed because the is_amp_endpoint() function cannot be called before the wp action, as it needs access to the queried object.

Testing instructions:

  1. Ensure Jetpack's masterbar module is active.
  2. Activate the AMP plugin v1.0.1
  3. Enable the new paired mode via the AMP settings.
  4. Navigate to a singular post.

Without the changes in this PR, there should be two PHP notices being raised:

( ! ) Notice: Trying to get property 'post_type' of non-object in .../amp/includes/class-amp-post-type-support.php on line 80

( ! ) Notice: Trying to get property 'ID' of non-object in .../amp/includes/class-amp-post-type-support.php on line 101

See also ampproject/amp-wp#1794 which will catch the incorrect usage of is_amp_endpoint().

Proposed changelog entry for your changes:

  • Fix AMP integration with masterbar module.

@westonruter westonruter requested a review from a team January 4, 2019 18:49
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Jan 4, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 10, 2019.
Scheduled code freeze: January 3, 2019

Generated by 🚫 dangerJS against a4caf27

@jeherve jeherve added this to the 6.9 milestone Jan 7, 2019
@jeherve jeherve added Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations AMP labels Jan 7, 2019
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jan 7, 2019

The changes in #10945 caused some issues with the Masterbar when on the Jetpack dashboard pages, as reported in #11081.

This PR now stops the masterbar from loading at all on those pages. It works well on other pages, though.

jeherve
jeherve reviewed Jan 7, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It seems to work better with admin_bar_init, although I can't really explain why.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jan 7, 2019
@jeherve @westonruter
Initialize masterbar at admin_bar_init action instead of wp for compa…
a4caf27 
…t in wp-admin

Co-Authored-By: westonruter <westonruter@google.com>
@westonruter
Copy link
Copy Markdown
Contributor Author

westonruter commented Jan 7, 2019

It seems to work better with admin_bar_init, although I can't really explain why.

I think the reason is that in the admin, the wp action happens after admin_bar_init has already happened, whereas on the frontend the order is reversed. Were you seeing the masterbar broken in the admin but working on the frontend?

@jeherve jeherve removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jan 7, 2019
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jan 7, 2019

Were you seeing the masterbar broken in the admin but working on the frontend?

Yep, that's what was happening. Thanks for digging into it!

@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 7, 2019
jeherve
jeherve approved these changes Jan 7, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work well on my end. 👍 🚢

@westonruter
Copy link
Copy Markdown
Contributor Author

westonruter commented Jan 7, 2019

Should this be assigned to me now? I can't merge it 😄

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jan 8, 2019

No worries :) Someone else will come around, review and merge :)

oskosk
oskosk approved these changes Jan 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeherve jeherve merged commit e68654b into Automattic:master Jan 8, 2019
@ghost ghost removed [Status] Needs Changelog [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 8, 2019
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jan 8, 2019

Cherry-picked to branch-6.9 in aa64a8c

jeherve pushed a commit that referenced this pull request Jan 8, 2019
@westonruter @jeherve
Fix AMP integration by deferring masterbar setup to wp action (#11088)
aa64a8c 
<!--- Provide a general summary of your changes in the Title above -->

Fixes #11082 
Fixes #11081

#### Changes proposed in this Pull Request:

* Defer masterbar setup until the `wp` action, instead of doing the setup at the `init` action. This is needed because the `is_amp_endpoint()` function cannot be called before the `wp` action, as it needs access to the queried object.

#### Testing instructions:
<!-- Please include detailed testing steps, explaining how to test your change. -->
<!-- Bear in mind that context you working on is not obvious for everyone.  -->
<!-- Adding "simple" configuration steps will help reviewers to get to your PR as quickly as possible. -->
<!-- "Before / After" screenshots can also be very helpful when the change is visual. -->

1. Ensure Jetpack's masterbar module is active.
2. Activate the AMP plugin v1.0.1
3. Enable the new paired mode via the AMP settings.
4. Navigate to a singular post.

Without the changes in this PR, there should be two PHP notices being raised:

```
( ! ) Notice: Trying to get property 'post_type' of non-object in .../amp/includes/class-amp-post-type-support.php on line 80

( ! ) Notice: Trying to get property 'ID' of non-object in .../amp/includes/class-amp-post-type-support.php on line 101
```

See also ampproject/amp-wp#1794 which will catch the incorrect usage of `is_amp_endpoint()`.

#### Proposed changelog entry for your changes:
<!-- Please do not leave this empty. If no changelog entry needed, state as such. -->

* Fix AMP integration with masterbar module.
ockham added a commit that referenced this pull request Jan 8, 2019
@ockham
Revert "Fix AMP integration by deferring masterbar setup to wp action (…
2fef098 
…#11088)"

This reverts commit e68654b.
@westonruter westonruter mentioned this pull request Feb 28, 2019
Open
@jeherve jeherve mentioned this pull request May 23, 2019
Closed
@obenland obenland mentioned this pull request Nov 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

+1 more reviewer

@oskosk oskosk oskosk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@westonruter westonruter

Labels

AMP Bug When a feature is broken and / or not performing as intended [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations

Projects

None yet

Milestone

6.9

Development

Successfully merging this pull request may close these issues.

5 participants

@westonruter @jetpackbot @jeherve @oskosk @matticbot