AdvancedCustomFields / acf

Advanced Custom Fields
http://advancedcustomfields.com/
835 stars 170 forks source link

acf_prepare_block className removed #577

Open thomasmb opened 2 years ago

thomasmb commented 2 years ago

In 5.11 it seems that the following code has been removed from in pro > blocks.php > acf_prepare_blocks()

// Replace className with wpClassName if it's set. This enables support for blocks API v2 filters.
if ( isset( $block['wpClassName'] ) ) {
    $block['className'] = $block['wpClassName'];
    unset( $block['wpClassName'] );
}

We've seen this causing issues on a few sites. Looks like a breaking change that should have been mentioned in update and the documentation?

lgladdy commented 2 years ago

Hey @thomasmb,

We did note this in the release blog post as "WordPress Blocks API v2 Updates" - and the changelog too.

It was a change we introduced in 5.10 but had to pause in 5.11 due to re-rendering race conditions that broke features like block styles. Essentially, we tried to support WordPress classes for the blocks like any other block as a property instead of markup, but that approach has too many downsides we don't think we can overcome.

We're planning on reintroducing support in another way in 5.12 though, so please take a look at #569 and give us any feedback our proposed way forward here.

thomasmb commented 2 years ago

Right, className part being removed was just implicit. Understood. Sorry

lgladdy commented 2 years ago

@thomasmb Yeh - className should still be set - but it won't contain the WordPress generated classes. We attempted to replace the "default" className with the expanded set of classes that the block editor returned to us.

StudioAl commented 2 years ago

I read the changelog and update, but I'm still not clear on the practical implications of what it means in regard to what's available at any given time in the block's array.

When I updated to 5.11, pretty much all of my blocks templates that were using $block['className'] broke — I was getting undefined index errors — className was no longer set (though it still was for some blocks). But you say it should always be set?

The issue was temporarily resolved last night, until I could look into it more, by switching to wpClassName, which, though not what I wanted, was, at the time, actually set in all of the blocks. But this morning, the client has attempted to edit a couple of those blocks and we're now getting an undefined index error for 'wpClassName' on those blocks.

I'm confused about what should be set and when, and why it seems to be inconsistent. Thanks.

lgladdy commented 2 years ago

@StudioAl Hey.. this is weird. $block['className'] should be set, no matter what version of ACF you're on. Even during 5.10.x when wpClassName was there too, className should have existed (but been a subset of wpClassName, as the additional WordPress added classes weren't present)

Any changes to these classes etc require a re-render of the block in the block editor, which triggers WordPress to update all the data on a block. This could be why you're seeing cases where blocks appear differently, as until you edit each one they're not updated.

That said, I think it's possible that a block's className could not be set, so you should support that condition. Outside of whatever weirdness has happened here, because those properties are build from the metadata WordPress saves in a block's comment - that can be edited, and if the className attribute is removed, it won't be present in your block either.

For now though, definitely code against className being the place where all classes will exist - if it's set and any exist. wpClassName was a short-lived internal helper property that was removed in 5.11, but will still appear on blocks that haven't been modified since they were modified on a 5.10.x build.

Edit: If you have any blocks which are still saying className isn't defined - could you send over the full code view of the block so we can see what properties it has set?

StudioAl commented 2 years ago

@lgladdy thanks, that's super helpful, and what I was suspecting. But I did expect that className would always be set, even if it were empty. Sounds like the best path forward is to go ahead and switch back to className and add checks before calling it. I'll post back if I run into more undefined index errors in the process.

StudioAl commented 2 years ago

I was able to look into this a little more, and it seems the undefined index error is due to className not setting if it is empty, which is perhaps as designed, and I my code should have accounted for that regardless.

The reason I likely never noticed is that className always populated with the Wordpress classes as well (so className was never empty), and this appears to have stopped — it's not returning the block classname or any style classes. Based on what you've said above, it should still be returning all of those, correct?

So, for example, if I've added a yellow background color, and a 'test' custom class, className would formerly return: wp-block-my-custom-block has-yellow-background-color test but now it just returns: test

Here's the WP code for a block with an undefined className index (no class names returned):

<!-- wp:acf/bookend {"id":"block_61807f9e1acb5","name":"acf/bookend","data":{"image":1849,"_image":"field_614abfa456a58","alignment":"right","_alignment":"field_614abfba56a59","width":"23","_width":"field_618036bb54af2","offset_v":"20","_offset_v":"field_617f5119197eb","offset_h":"13","_offset_h":"field_617f5176197ec","hide_on_mobile":"0","_hide_on_mobile":"field_618080f8ad7bf","hide_on_desktop":"0","_hide_on_desktop":"field_61808113ad7c0"},"align":"","mode":"preview"} /-->

Here's the WP code for a block where the className is set, but where it does not return the WP class names (the only class name returned is the custom class 'home-header'):

<!-- wp:acf/pageheader {"id":"block_6144e4af6e411","name":"acf/pageheader","data":{"image_l":"","_image_l":"field_60d6711a79113","image_r":"","_image_r":"field_60d6712879114","background_image":"","_background_image":"field_60d68e78feb8e"},"align":"","mode":"preview","className":"home-header","backgroundColor":"yellow","textColor":"white"} -->
<!-- wp:heading {"level":1,"align":"center","textColor":"black"} -->
<!-- wp:heading {"level":1,"align":"center","textColor":"black"} -->
<h1 class="has-black-color has-text-color">It’s time to<br> <em>love</em> local.</h1>
<!-- /wp:heading -->
lgladdy commented 2 years ago

@StudioAl The block classes you mentioned won't be returned as WordPress tries to apply them to the outer most HTML element inside the block comment - but ACF doesn't have that, as we use your template to render instead of inside the block comment like a normal block.

The only time className would be set would be if an additional class is added, or you use a block pattern or block style which saves the value to the className attribute. Essentially, we're currently passing all the raw internal WordPress attribute values into the template which you then use to apply those styles - but this isn't what's "supposed" to happen in the block editor, as WordPress turns all those internal attributes on the block comment into valid markup.

That's our end goal for here, as WordPress could make an internal change any of those attributes at any time and it could break ACF block templates - because they expect you to let the block editor render the markup.

StudioAl commented 2 years ago

@lgladdy OK, thank you for the clarity on that. Oof. So… I'm trying to figure out how to proceed from here to update my ACF Block templates (of which there are many), as the WP styles are absolutely crucial to the site rendering properly. Apologies if this is outside the scope of this Issue report, but — is there an alternative manual way to access the block's WP styles from within the block template, so I can manually apply them? I'm gathering, at this point, no, otherwise you'd be able to incorporate it into the $block array?

lgladdy commented 2 years ago

@StudioAl All the data WordPress uses to build those class names is available in the $block array, but as the internal helpers.

There are things like 'wp-block-acf-$blockname' which would require you to manually add something like 'wp-block-acf-'.$block['name'], or if you're using a background colour, that would be in something like $block['backgroundColor'], so you could detect that and output 'has-background-color'. This is how you had to output those classes pre ACF 5.11, so most people already had existing code to do this for their themes.

If there's any specific classes missing now that you need help figuring out how to build the name from the other attributes, let me know!

StudioAl commented 2 years ago

@lgladdy Doh, sorry! I could have easily seen those in in the $block array had I looked hard enough. Yes, I must have come into this at the worst time — the first time I've used ACF with blocks was between now and 5.11, so I was just using what appeared available! Thanks for your patience explaining to me. Though labor intensive, I have what I need to future-proof this now.

metropoliscreative commented 2 years ago

This is how you had to output those classes pre ACF 5.11, so most people already had existing code to do this for their themes.

No, formatted color classes were included in the $block['className'] string before ACF 5.11. All of my blocks were built this way.

I'm not understanding this "in-between" phase. It seems that wpClassName "works", and includes all of the formatted color classes along with any additional classes added by the user, but you're advising not to use this because you're going to remove it?

Can you please clarify how you recommend rendering block classes that will work both now, and after ACF 5.12?

lgladdy commented 2 years ago

@metropoliscreative wpClassName will only be present on blocks added to a page during ACF 5.10. If you add a block now (or in ACF <5.10) it won't be present at all.

Could you confirm what class you were expected to be present, and which supports feature you're using to select the colours - and I'll do some testing on this?

metropoliscreative commented 2 years ago

@lgladdy Understood, and thanks for your quick reply. Before ACF 5.11, the className string did include all of the formatted color classes that were selected from the blocks Color settings (by setting block's color supports to true). This included text color, background color and gradient classes.

With these class names automatically removed after updating I'm trying to figure out how to fix all of our custom blocks across all of our sites quickly, but am trying to understand how the 5.12 changes will also affect this issue. From what I can gather, you're recommending we generate all of the block's color classes from scratch using the data in the block's attributes, but it sounds like 5.12 will introduce a "block props" wrapper automatically with all of these classes applied, unless we opt out?

Until that wrapper was introduced, the className functionality should not have been changed.

lgladdy commented 2 years ago

@metropoliscreative Cheers, when you talk about before ACF 5.11, are you including ACF 5.9.x in that too? We're aware those classes were present for anyone who built on ACF 5.10, but we don't believe they where there in ACF 5.9.x. We reverted the change that added them in ACF 5.10 as it introduced too many issues with other block editor features, such as block styles - and it only applied to modified or new blocks added during that time ACF 5.10 was installed.

We won't implement any "opt-out" changes that change how blocks work fundamentally, or will require any code changes from you - anything like that will be opt in (likely via a new block version parameter you can set on your block definition) - so anything you code now will work just fine for as many future versions as possible (unless WordPress change something in core which forces a change).

metropoliscreative commented 2 years ago

@lgladdy You're right, just 5.10

lgladdy commented 2 years ago

@metropoliscreative Ah, okay - that makes sense. I gave a bit more technical on why we have to revert this change here, but it is our goal that all these classes will be available to a block as you'd kinda expect it to work this way - just like it does with native blocks; but we need a little more work to get there!

chingologram commented 1 year ago

Hi to everyone. Just bumping on this issue as I also came accross this issue when updating to newer ACF versions. Also, got me thinking if we could use a filter before acf_prepare_block to allow users to manipulate block data. Something like acf/before_prepare_block ? There is the possibility to use render_callback but it has to be specified every time for each block and cannot be used in conjunction with render_template.