Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug Fix: Parsing boolean block attributes like in CoreVideo always return false. #138

Merged
merged 4 commits into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/spicy-tables-cross.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@wpengine/wp-graphql-content-blocks": patch
---

Bug Fix. Boolean block attributes no longer always resolve as false.
13 changes: 9 additions & 4 deletions includes/Blocks/Block.php
Original file line number Diff line number Diff line change
Expand Up @@ -286,10 +286,15 @@ private function resolve_block_attributes( $block, $attribute_name, $attribute_c

break;
}//end switch

// if type is set to integer, get the integer value of the attribute.
if ( 'integer' === $attribute_config['type'] ) {
$value = intval( $value );
// Post processing of return value based on configured type
switch ( $attribute_config['type'] ) {
case 'integer':
$value = intval( $value );
break;
case 'boolean':
// Only false when value is not null or actually false
$value = ! ( false === $value || is_null( $value ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to be concerned with other falsey values like an empty string?

Im not too familiar with the Block Attribute type validation, but WP is usually horrible at strict booleans.

Copy link
Member Author

@theodesp theodesp Jul 26, 2023

Choose a reason for hiding this comment

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

I think this is specifically targeted for empty strings. An empty string in the context of parsing a boolean attribute like
<video autoplay /> here autoplay="" which according to the HTML spec it's enabled. So in that case we return true. So unless the value is specifically false or null it is considered truthy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember that attributes here refer to all block attributes, and just not HTML attributes, so I wouldn't rely on that assumption that the underlying WordPress parser is making that distinction when it comes to the HTML spec.

Any while you've labeled this PR (and the associated test) as specific to CoreVideo, you're actually adding bool handling to all Block attributes plus passing them through helper libraries like DiDom.

It should be easy enough to test and confirm either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the code to handle null returns now. This should work as expected.

break;
}

return $value;
Expand Down
95 changes: 95 additions & 0 deletions tests/unit/blocks/CoreVideoTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
<?php

namespace WPGraphQL\ContentBlocks\Unit;

final class CoreVideoTest extends PluginTestCase {
public $instance;
public $post_id;

public function setUp(): void {
parent::setUp();
global $wpdb;

$this->post_id = wp_insert_post(
array(
'post_title' => 'Post Title',
'post_content' => preg_replace(
'/\s+/',
' ',
trim(
'
<!-- wp:video {"id":1636} -->
<figure class="wp-block-video"><video autoplay controls loop poster="http://mysite.local/wp-content/uploads/2023/05/pexels-egor-komarov-14420089-scaled.jpg" preload="auto" src="http://mysite.local/wp-content/uploads/2023/07/pexels_videos_1860684-1440p.mp4" playsinline></video></figure>
<!-- /wp:video -->
'
)
),
'post_status' => 'publish',
)
);
}

public function tearDown(): void {
// your tear down methods here
parent::tearDown();
wp_delete_post( $this->post_id, true );
}

public function test_retrieve_core_video_attributes() {
$query = '
fragment CoreVideoBlockFragment on CoreVideo {
attributes {
align
anchor
autoplay
tracks
muted
caption
preload
src
playsInline
controls
loop
poster
id
}
}

query GetPosts {
posts(first: 1) {
nodes {
databaseId
editorBlocks {
name
...CoreVideoBlockFragment
}
}
}
}
';
$actual = graphql( array( 'query' => $query ) );
$node = $actual['data']['posts']['nodes'][0];

// Verify that the ID of the first post matches the one we just created.
$this->assertEquals( $this->post_id, $node['databaseId'] );
// There should be only one block using that query when not using flat: true
$this->assertEquals( count( $node['editorBlocks'] ), 1 );
$this->assertEquals( $node['editorBlocks'][0]['name'], 'core/video' );

$this->assertEquals( $node['editorBlocks'][0]['attributes'], [
'align' => null,
'anchor' => null,
'autoplay' => true,
'tracks' => '[]',
'muted' => false,
'caption' => '',
'preload' => 'auto',
'src' => 'http://mysite.local/wp-content/uploads/2023/07/pexels_videos_1860684-1440p.mp4',
'playsInline' => true,
'controls' => true,
'loop' => true,
'poster' => 'http://mysite.local/wp-content/uploads/2023/05/pexels-egor-komarov-14420089-scaled.jpg',
'id' => 1636.0,
]);
}
}
Loading