Embed block: fix embedding stories with plain permalinks #5561
Conversation
swissspidy
added
Type: Bug
|
Size Change: +89 B (0%) Total Size: 1.5 MB ℹ️ View Unchanged
|
|
@spacedmonkey I know, on it already |
Codecov Report
@@ Coverage Diff @@
## main #5561 +/- ##
==========================================
+ Coverage 84.48% 84.74% +0.25%
==========================================
Files 1002 1002
Lines 17647 17668 +21
==========================================
+ Hits 14909 14972 +63
+ Misses 2738 2696 -42
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| } | ||
|
|
||
| if ( Story_Post_Type::POST_TYPE_SLUG !== $post->post_type ) { | ||
| return false; | ||
| if ( ! $post_id ) { |
There was a problem hiding this comment.
| if ( ! $post_id ) { | |
| else { |
Doesn't it make more sense to use an else here?
There was a problem hiding this comment.
The result is exactly the same, so just a matter of preference for coding style and readability.
Aside: PHPMD by default complains about using else branches, but we've disabled that.
| $this->set_permalink_structure( '/%postname%/' ); | ||
| $this->go_to( get_permalink( self::$story_id ) ); | ||
| } | ||
|
|
||
| public function tearDown() { | ||
| $this->set_permalink_structure( '' ); |
There was a problem hiding this comment.
Can you explain this change?
There was a problem hiding this comment.
This is just some regular cleanup for these tests. The setUp calls $this->set_permalink_structure( '/%postname%/' );, so it's important to reset that again; otherwise it will affect other tests in other files.
| @@ -76,6 +76,13 @@ public static function wpSetUpBeforeClass( $factory ) { | |||
| set_post_thumbnail( self::$story_id, $poster_attachment_id ); | |||
| } | |||
|
|
|||
| public function tearDown() { | |||
There was a problem hiding this comment.
Can you explain this / add a comment here.
There was a problem hiding this comment.
This is just some regular cleanup for these tests. Some tests call $this->set_permalink_structure( '/%postname%/' );, so it's important to reset that again; otherwise it will affect other tests in other files.
| $path = explode( '/', ltrim( $url_parts['path'], '/' ) ); | ||
| $path = reset( $path ); | ||
| $path = count( $path ) > 2 ? reset( $path ) : false; |
There was a problem hiding this comment.
| $path = count( $path ) > 2 ? reset( $path ) : false; | |
| $path = count( $path ) > 2 ? reset( $path ) : ''; |
Why not default to an empty string?
There was a problem hiding this comment.
Doesn't matter too much, just needs to be falsey for the check below.
There was a problem hiding this comment.
Empty string is a default value of get_networks.
There was a problem hiding this comment.
Right, but $path is only used below when if ( $path && $network instanceof \WP_Network )` evaluates to true. So this won't make a difference.
I can change it though, no problem. Prevents accidental bugs in the future 👍 Just wanted to point that out :)
Summary
Fixes embedding local stories with plain permalinks.
Relevant Technical Choices
url_to_postid()does not recognize plain permalinks likehttps://example.com/?web-story=my-story.To-do
User-facing changes
N/A
Testing Instructions
Fixes #5560