Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#34971 closed enhancement (fixed)

Embeds: Embedding static front page does not work

Reported by: thomaslhotta's profile thomaslhotta Owned by: swissspidy's profile swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Hi,

Just started playing around with the new embed feature and I really like it. However I discovered that it is not possible to embed the front page of a blog when it is set to a static page.

WP_oEmbed_Controller::get_item does not find the post id for the static front page. It also seems like WP_Query cannot handle this situation either as any request using ?embed=true to the front page returns the blog archive page instead without using the embed template.

I guess embedding single posts/pages is the use case for the majority of users, but having it work for front pages would also be a nice way for promoting individual blogs on a multisite install.



Attachments (5)

34971.diff (10.5 KB) - added by swissspidy 8 years ago.
34971.2.diff (14.3 KB) - added by swissspidy 8 years ago.
34971.3.diff (17.6 KB) - added by swissspidy 8 years ago.
34971-no-alt-tag.diff (549 bytes) - added by peterwilsoncc 8 years ago.
34971.4.diff (17.3 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (30)

#1 @swissspidy
8 years ago

Interesting use case.

The embed endpoint is only added to permalinks, not the front page, so adding ?embed=true won't work.

The other part would be in url_to_postid, which perhaps should check for the front page setting.

#2 @thomaslhotta
8 years ago

I did some further testing of this. The front page has no endpoint as you say, but adding ?embed=true does pass the required query var to WP_Query. However it returns the blog archive page instead of the front page. I tracked the problem down to line 2617 in query.php:

if ( $this->is_home && ( empty($this->query) || $q['preview'] == 'true') && ( 'page' == get_option('show_on_front') ) && get_option('page_on_front') ) {
     $this->is_page = true;
     $this->is_home = false;
     $q['page_id'] = get_option('page_on_front');
}

empty($this->query) prevents the front pages from being set as it contains the embed query var. I tried commenting it out but there are also other things going wrong further down the line.

#3 @swissspidy
8 years ago

adding ?embed=true does pass the required query var to WP_Query. However it returns the blog archive page instead of the front page.

Related: #25143

@swissspidy
8 years ago

#4 @swissspidy
8 years ago

  • Keywords has-patch has-unit-tests added

Definitely something we should fix somehow.

Currently we show oEmbed discovery links on a static front page, but embedding does not work. I wouldn't necessarily support embedding the front page in general (i.e. when it's showing the recent posts), but this should be fixed somehow.

34971.diff does the following:

  • Adds the necessary rewrite rules for supporting /embed/ for static front pages (not the front page in general!)
  • Adds support for detecting the static front page in url_to_postid()
  • Adds unit tests

Seems like 4.4.1 material to me. @pento What do you think?

Last edited 8 years ago by swissspidy (previous) (diff)

#5 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.4.1

#6 follow-up: @pento
8 years ago

How does this patch affect pages with the slug embed?

#7 in reply to: ↑ 6 @swissspidy
8 years ago

Replying to pento:

How does this patch affect pages with the slug embed?

Well, at the moment this would prevent you from visiting a page with that slug. We'd need to add an exception to wp_unique_post_slug() for embed. Looks like something we missed when developing the feature.

However, I don't know yet how we can do that retroactively. Suggestions welcome.

@swissspidy
8 years ago

#8 @pento
8 years ago

Currently, a page with the slug embed displays correctly, so we need this patch to continue doing that.

It's kind of an edge case, but I think it'll be more common than when we had the same discussion for wp-json.

I'm leaning towards having the embed URL be the full page URL - if you set sample-page as the front page, then the embed URL is /sample-page/embed/. The downside is that this method won't notice if the frontpage page is changed. Maybe only do that if the embed slug is being used, to reduce the likelihood of it being an issue?

I have no good solution at the moment. I'll give it some more thought, I'm open to any suggestions.

#9 follow-up: @peterwilsoncc
8 years ago

Spit balling different rules for embedding the home page:

  • always ugly /?embed=true, biggest down side is it goes against the WordPress preference for pretty permalinks
  • /embed/ falling back to the ugly version if the slug exists. Biggest downside is a page with the embed slug being added after someone copies the HTML code into their site.
  • /_embed/
  • /wp-embed/

I don't really like exceptions/unique rules either. ¯\_(ツ)_/¯

Last edited 8 years ago by peterwilsoncc (previous) (diff)

#10 in reply to: ↑ 9 @pento
8 years ago

Replying to peterwilsoncc:

  • /embed/ falling back to the ugly version if the slug exists. Biggest downside is a page with the embed slug being added after someone copies the HTML code into their site.

Let's do this. New pages can't be created with embed as the slug with 34971.2.diff.

Last edited 8 years ago by pento (previous) (diff)

@swissspidy
8 years ago

#11 @swissspidy
8 years ago

34971.3.diff is a new patch that makes get_post_embed_url() fall back to ugly permalinks when the path conflicts with another post. Adds two new unit tests for it.

Uses get_page_by_path() after stripping of the home_url() because url_to_postid strips off query vars like embed. There's probably a nicer solution for that, so let me know how it can be improved.

#12 @swissspidy
8 years ago

  • Summary changed from Embedding static front pages on multisite does not work to Embeds: Embedding static front page does not work

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 years ago

#14 @jorbin
8 years ago

  • Owner set to pento
  • Status changed from new to assigned

#15 follow-up: @dd32
8 years ago

This feels like 4.5 material. As it's not a regression, nor a significant "bug" (It's simply unsupported), I don't see the need to put it into a point release.

#16 @johnbillion
8 years ago

  • Milestone changed from 4.4.1 to 4.5
  • Type changed from defect (bug) to enhancement

#17 in reply to: ↑ 15 ; follow-up: @peterwilsoncc
8 years ago

Replying to dd32:

This feels like 4.5 material. As it's not a regression, nor a significant "bug" (It's simply unsupported), I don't see the need to put it into a point release.

It's buggy in as much as the HTML header includes oembed link tags that are a dead end, at the very least would be super to get 34971-no-alt-tag.diff in the 4.4 branch to prevent the 404s.

#18 in reply to: ↑ 17 @dd32
8 years ago

Replying to peterwilsoncc:

Replying to dd32:

This feels like 4.5 material. As it's not a regression, nor a significant "bug" (It's simply unsupported), I don't see the need to put it into a point release.

It's buggy in as much as the HTML header includes oembed link tags that are a dead end, at the very least would be super to get 34971-no-alt-tag.diff in the 4.4 branch to prevent the 404s.

That part is definitely a bug worth fixing in 4.4.1.

#19 @peterwilsoncc
8 years ago

Forked the discovery tag removal to #35194 to keep this on the enhancement.

Last edited 8 years ago by peterwilsoncc (previous) (diff)

#20 @pento
8 years ago

  • Keywords needs-refresh added
  • Owner pento deleted

The patch will need to be updated to revert the behaviour of [36059].

@swissspidy
8 years ago

#21 @swissspidy
8 years ago

  • Keywords needs-refresh removed

34971.4.diff is an updated patch that reverts the behaviour of [36059].

#22 @swissspidy
8 years ago

  • Owner set to pento

#23 @swissspidy
8 years ago

  • Keywords commit added

#24 @swissspidy
8 years ago

  • Owner changed from pento to swissspidy
  • Status changed from assigned to accepted

#25 @swissspidy
8 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 36307:

Embeds: Allow embedding static front pages and pages having a child page with an embed slug.

This makes embed a special slug that can't be used for new pages/posts. When https://example.com/foo/embed/ is an existing page, embeds fall back to https://example.com/foo/?embed=true.
Adds unit tests.

Fixes #34971.

Note: See TracTickets for help on using tickets.