Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#34971 closed enhancement (fixed)

Embeds: Embedding static front page does not work

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

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 11 years ago.
34971.2.diff (14.3 KB ) - added by swissspidy 11 years ago.
34971.3.diff (17.6 KB ) - added by swissspidy 11 years ago.
34971-no-alt-tag.diff (549 bytes ) - added by peterwilsoncc 11 years ago.
34971.4.diff (17.3 KB ) - added by swissspidy 11 years ago.

Download all attachments as: .zip

Change History (30)

#1 @swissspidy
11 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
11 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
11 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
11 years ago

#4 @swissspidy
11 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 11 years ago by swissspidy (previous) (diff)

#5 @SergeyBiryukov
11 years ago

  • Milestone Awaiting Review4.4.1

#6 follow-up: @pento
11 years ago

How does this patch affect pages with the slug embed?

#7 in reply to: ↑ 6 @swissspidy
11 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
11 years ago

#8 @pento
11 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
11 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 11 years ago by peterwilsoncc (previous) (diff)

#10 in reply to: ↑ 9 @pento
11 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 with can't be created with embed as the slug with 34971.2.diff.

Version 0, edited 11 years ago by pento (next)

@swissspidy
11 years ago

#11 @swissspidy
11 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
11 years ago

  • Summary Embedding static front pages on multisite does not workEmbeds: Embedding static front page does not work

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


11 years ago

#14 @jorbin
11 years ago

  • Owner set to pento
  • Status newassigned

#15 follow-up: @dd32
11 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
11 years ago

  • Milestone 4.4.14.5
  • Type defect (bug)enhancement

#17 in reply to: ↑ 15 ; follow-up: @peterwilsoncc
11 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
11 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
11 years ago

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

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

#20 @pento
11 years ago

  • Keywords needs-refresh added
  • Owner pento removed

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

@swissspidy
11 years ago

#21 @swissspidy
11 years ago

  • Keywords needs-refresh removed

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

#22 @swissspidy
11 years ago

  • Owner set to pento

#23 @swissspidy
11 years ago

  • Keywords commit added

#24 @swissspidy
11 years ago

  • Owner changed from pento to swissspidy
  • Status assignedaccepted

#25 @swissspidy
11 years ago

  • Resolutionfixed
  • Status acceptedclosed

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.