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

Filter to allow plugins override block attributes #11730

Closed

Conversation

kkarpieszuk
Copy link

@kkarpieszuk kkarpieszuk commented Nov 11, 2018

Description

Closes #8912, #11195 (related but more specific proposals).

Add a filter so plugins and themes can override block attributes.

Why is this needed?

Dynamic blocks, rendered server side keep their values in block attributes. Some of those attributes might be texts printed in the front-end to be read by people. This kind of strings should be available for translation.
We are preparing in WPML code to make it translatable, but we need to hook somewhere somewhere in Gutenberg to filter values. This place seems to be most appropriate for this.

How has this been tested?

This has been tested in WPML with following code example (to translate dynamic strings created with ACF Blocks beta).

add_filter( 'block_prepared_attributes', 'block_attributes_filter', 1, 10 );
function block_attributes_filter( $attributes, $block_type ) {
    if ( $block_type->name === 'my-plugin/my-block' &&  isset( $attributes['data'] ) ) {
            $original_post_id = apply_filters( 'wpml_object_id', get_the_ID(), get_post_type(), true, apply_filters( 'wpml_default_language', null) );
            foreach ( $attributes['data'] as $name => $value ) {
                $string_name = WPML_Gutenberg_Strings_In_Block::get_string_id($attributes['name'], $value);
                $translated = apply_filters( 'wpml_translate_string',
                    $value,
                    $string_name,
                    array('kind' => 'Gutenberg', 'name' => $original_post_id) );
                $attributes['data'][$name] = $translated;
            }
        }

        return $attributes;
}

Types of changes

Add a new filter

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

* @param array $prepared_attributes block attributes
*/

$prepared_attributes = apply_filters( 'block_attributes', $prepared_attributes );
Copy link
Member

Choose a reason for hiding this comment

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

IMO $this should be passed to the filter as well. Otherwise one has no idea which block type the attributes are for.

Copy link
Member

@gziolo gziolo Nov 12, 2018

Choose a reason for hiding this comment

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

Can't it pass the block name instead of $this? What other information plugins would need? It feels like there is no need to pass the block type object by reference.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I second idea to pass the whole object. Mind it, plugin authors will filter not only core Blocks but also many 3rd party with who knows what kind of data. Sometimes it will be helpful to have bigger picture inside of the hook.

But finally it is up to you, I can change but please decide :)

Choose a reason for hiding this comment

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

+1 It will be more useful to be given the whole object here instead of the "block type name".

@swissspidy
Copy link
Member

I left some comments because the added code does not yet follow the WordPress PHP documentation standards.

@kkarpieszuk
Copy link
Author

Thank you @swissspidy I adjusted the code according your suggestions

@mtias mtias requested a review from gziolo November 12, 2018 19:45
@mtias mtias added [Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience labels Nov 12, 2018
@gziolo gziolo requested a review from pento November 12, 2018 20:28
@gziolo gziolo added this to the 4.4 milestone Nov 12, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

@pento, I didn't follow PHP based hooks for Gutenberg. Could you double check this one?

The use case described makes sense. What I'm not quite sure is how it relates to #8912 which also tries to integrate WPML plugin using reusable blocks:

We are preparing in WPML code to make it translatable, but we need to hook somewhere somewhere in Gutenberg to filter values. This place seems to be most appropriate for this.

lib/class-wp-block-type.php Outdated Show resolved Hide resolved
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This needs to be fixed:

FILE: /app/lib/class-wp-block-type.php

FOUND 2 ERRORS AFFECTING 2 LINES

154 | ERROR | [x] Whitespace found at end of line
| | (Squiz.WhiteSpace.SuperfluousWhitespace.EndLine)
156 | ERROR | [x] Whitespace found at end of line
| | (Squiz.WhiteSpace.SuperfluousWhitespace.EndLine)

PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY

lib/class-wp-block-type.php Outdated Show resolved Hide resolved
@gziolo
Copy link
Member

gziolo commented Nov 16, 2018

Can we also consolidate changes proposed in #8912 to avoid introducing another more targeted hook:
https://github.com/WordPress/gutenberg/pull/8912/files#diff-04845e982a5b395a941d0542ea071a9cR33

I guess, we would have to pass a post id or even a full object.

@gziolo
Copy link
Member

gziolo commented Nov 16, 2018

I think we should also propose the same change in Trac to ensure it lands in WordPress core before RC starts.

@kkarpieszuk
Copy link
Author

about passing post id: I am not sure if I understood, but I think post is not available in this scope (it is in globals, this is what you meant? to take it from global? if so, it could be redundant: hooked filter will have access to global as well so no need to pass it)

about Trac: I don't know how to propose it there, could you give me a hand?

@gziolo gziolo added the [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later label Nov 19, 2018
@catehstn
Copy link

This does not seem ready to merge in 4.5, so moving to 4.6.

@catehstn catehstn modified the milestones: 4.5, 4.6 Nov 19, 2018
@kkarpieszuk
Copy link
Author

@gziolo if this is postponed to 4.6, should I still add similar change to wordpress repository? if yes, could you guide me how to (especially where exactly)?

@mtias mtias modified the milestones: 4.6, 4.7 Nov 22, 2018
@gziolo
Copy link
Member

gziolo commented Nov 26, 2018

5.0 RC is out so this won't be included for the initial 5.0 release. This is a feature request and it's now planned for the first following release 5.0.1 - as of today (that's why it is included in the plugin's 4.7 milestone).

@kkarpieszuk
Copy link
Author

Thanks @gziolo Do I need to to take any action or just wait for it?

@gziolo
Copy link
Member

gziolo commented Nov 26, 2018

It is still not clear what the process is going to look like after 5.0 is out. It probably makes sense to open a ticket in Trac and propose the changes directly in WordPress Core. @swissspidy and @pento should know better how to proceed.

@kkarpieszuk
Copy link
Author

@swissspidy / @pento could you advice how to copy this ticket to trac?

@swissspidy
Copy link
Member

@kkarpieszuk Sure!

Since WP_Block_Type is in core, a ticket on Trac definitely makes sense.

You can create a new ticket at https://core.trac.wordpress.org/newticket. If you don't have a WordPress.org account yet, you can sign up at https://login.wordpress.org/.

As for the ticket, make sure the type is "enhancement" and the version is 5.0.

Creating a patch is a little bit different than creating a PR here on GitHub.

You can clone the Git repository from develop.git.wordpress.org. Make sure to check out the 5.0 branch. Then, apply the changes from this PR to WP_Block_Type in core.

Make sure to change the @since version to 5.0.1.

Also, I'd replace Filter to allow plugins to override Block attributes. with Filters the prepared block attributes before rendering the current block..

After that, create a patch using git diff --no-prefix and upload it to the new Trac ticket.

You can include the same description from this PR when creating the ticket. You can also link to this PR for full context.

Let me know if you need any help during the whole process! I'll definitely keep an eye open on new tickets :-)

@kkarpieszuk
Copy link
Author

Hi @swissspidy thank you for detailed instructions but I can't connect to git server. Is the url correct? What protocol should I use?

this is what I tried:

konrad@konrad-TOSHIBA:~/tmp/wpgut$ git clone git://develop.git.wordpress.org
Cloning into 'develop.git.wordpress.org'...
fatal: No path specified. See 'man git-pull' for valid url syntax
konrad@konrad-TOSHIBA:~/tmp/wpgut$ git clone ssh://develop.git.wordpress.org
Cloning into 'develop.git.wordpress.org'...
fatal: No path specified. See 'man git-pull' for valid url syntax
konrad@konrad-TOSHIBA:~/tmp/wpgut$ git clone https://develop.git.wordpress.org
Cloning into 'develop.git.wordpress.org'...
fatal: unable to access 'https://develop.git.wordpress.org/': SSL: certificate subject name (*.svn.wordpress.org) does not match target host name 'develop.git.wordpress.org'
konrad@konrad-TOSHIBA:~/tmp/wpgut$ 

@swissspidy
Copy link
Member

@kkarpieszuk I think you need a trailing slash, e.g. git clone git://develop.git.wordpress.org/ or git clone https://develop.git.wordpress.org/

@kkarpieszuk
Copy link
Author

@swissspidy for https I didn't make any difference, but for ssh:// there is some progress. Looks like I need to have access granted, but how?

konrad@konrad-TOSHIBA:~/tmp/wpgut$ git clone ssh://develop.git.wordpress.org/
Cloning into 'develop.git.wordpress.org'...
ssh: connect to host develop.git.wordpress.org port 22: Connection timed out
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
konrad@konrad-TOSHIBA:~/tmp/wpgut$ 

I tried git clone ssh://[email protected]/ (kkarpieszuk is my wordpress.org login) but with the same result

@swissspidy
Copy link
Member

@kkarpieszuk Sorry you have all these troubles. I don't think specific access is needed. But in any case, there's a mirror at https://github.com/wordpress/wordpress-develop/ that one can use. This should work for sure :-)

@kkarpieszuk
Copy link
Author

@swissspidy thank you, done (I hope)! :) Could you review if everything is done correctly?

@swissspidy
Copy link
Member

Closing in favor of https://core.trac.wordpress.org/ticket/45451.

@swissspidy swissspidy closed this Dec 2, 2018
@mitchiru
Copy link

thanks a lot!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants