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

New Embed Variations Break allowed_block_types() Functionality. #27913

Open
timnolte opened this issue Dec 28, 2020 · 14 comments
Open

New Embed Variations Break allowed_block_types() Functionality. #27913

timnolte opened this issue Dec 28, 2020 · 14 comments
Labels
[Block] Embed Affects the Embed Block Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended

Comments

@timnolte
Copy link
Contributor

timnolte commented Dec 28, 2020

Describe the bug
Since WordPress changed the Embed blocks to all variations of the core/embed block they are now no longer being allowed individually by using the allowed_block_types() hook in previous versions of WP.

To reproduce
Steps to reproduce the behavior:

function my_allowed_block_types( $allowed_block_types, $post ) {

	if ( 'post' === $post->post_type ) {
		return array(
			'core/image',
			'core/paragraph',
			'core/heading',
			'core/list',
			'core/separator',
			'core/shortcode',
			'core/embed',
			'core-embed/vimeo',
		);
	}

	return $allowed_block_types;

}
// Hook function into allowed block type filter.
add_filter( 'allowed_block_types', 'my_allowed_block_types', 10, 2 );

Expected behavior
Embed variations are all distinct things that should be allowed to be whitelisted for use.

Editor version (please complete the following information):

  • WordPress version: 5.6.x
  • Does the website has Gutenberg plugin installed, or is it using the block editor that comes by default? [e.g: "gutenberg plugin", "default"] default
  • If the Gutenberg plugin is installed, which version is it? [e.g., 7.6] N/A
@annezazu annezazu added [Block] Embed Affects the Embed Block [Type] Bug An existing feature does not function as intended Needs Technical Feedback Needs testing from a developer perspective. labels Dec 29, 2020
@ntsekouras
Copy link
Contributor

Hey @timnolte - you could unregister any block variation you don't want to use (unregisterBlockVariation).

@timnolte
Copy link
Contributor Author

@ntsekouras the problem is that I want an explicit allow list. When developing client sites we are not always styling 100% of all Core blocks. Additionally, what you list is a totally different code base, JavaScript, so now I must maintain block controls 8n 2 different code bases. This is way more to maintain and now causes problems for where to go to maintain a single allowed block list. I should also point out that this breaks sites that have already been using the allowed_block_types hooks.

@ItsMarioSouza
Copy link

I agree with @timnolte. We have a similar setup for our Clients, a single explicit allowed list in PHP. Updating to WP 5.6 prevented our Clients from adding any embeds since we only allowed certain ones in the allowed_block_types hook.

@chthonic-ds
Copy link
Contributor

I share the same reasons as @timnolte states above for using an explicit allow list.

Targeting unrequired block variations by unregistering them would also mean having to manually update this deny list every time future releases introduce new variations.

@ntsekouras
Copy link
Contributor

Okay, I get what you mean. Block variations API is a powerful one, but is expanding right now.. There are some other similar issues about the allowedBlocks that mention similar restrictions. I'll check a bit more about expanding allowedBlocks with block variations..

@gherrink
Copy link

gherrink commented Feb 2, 2021

@chthonic-ds I had the same problem, that wen new variants will be added, they would be available. So as workaround i used getBlockVariants to get a list of all available variants and whitelist array of variants I want to allow. Something like this

import domeReady from '@wordpress/dom-ready';
import { getBlockVariations, unregisterBlockVariation } from '@wordpress/blocks';

domeReady(() => {
  const allowedEmbedVariants = ['youtube'];

  getBlockVariations('core/embed').forEach(variant => {
    if(!allowedEmbedVariants.includes(variant.name)) {
      unregisterBlockVariation('core/embed', variant.name);
    }
  });
});

This will hide all variants but not youtube and the main implementation of embed. The problem is that you sill can use the main implementation of embed to embed everything you want. @ntsekouras is there a way to to change this?

@ntsekouras
Copy link
Contributor

ntsekouras commented Feb 2, 2021

This will hide all variants but not youtube and the main implementation of embed. The problem is that you sill can use the main implementation of embed to embed everything you want. @ntsekouras is there a way to to change this?

This is really interesting! I'll look into it more :)

What do you think the behaviour should be on pasting such a link? Example unregister youtube and paste a youtube link.

@gherrink
Copy link

gherrink commented Feb 4, 2021

What do you think the behaviour should be on pasting such a link? Example unregister youtube and paste a youtube link.

When unregistering youtube I don't want that embedding of youtube links is possible.
I'm form Germany and GDPR is a big problem. For each embed you need to delay the loading untill the visitor allows it displaying this specific embed type. I want to control what embeds the editor is going to use. E.g. I've defined content blocking for only youtube then I don't want content from facebook on my site.

@chrisvanpatten
Copy link
Member

@ntsekouras I think the expected behavior would be that the link is just visible as text inside a paragraph, rather than being embedded.

@gherrink
Copy link

@ntsekouras I think the expected behavior would be that the link is just visible as text inside a paragraph, rather than being embedded.

@chrisvanpatten yes something like that. Also a info "This is not a valid embed URL. The URL will only displayed as Text." for the user how insert a not allowed embed URL would be nice too.

@matthewfarlymn
Copy link

I understand these can be disabled/enabled through JS but when enabling/disabling based on user->role the hook is much more beneficially.

Does anyone know how to accomplish a user->role based approach in JS?

@steveangstrom
Copy link

What exactly is the point of having PHP and a filter of "allowed_block_types_all"
if it doesn't work?

In the time this bug has been live the filter itself was deprecated and replaced, and left broken!

If the "answer " is to unhook each unwanted block in JS, then why did the PHP hook even change in WP5.8 ?

Why aren't core devs honest that they consider PHP sites as "dull" of no concern.

Be honest, its all about the latest shiny nonsense. Get ready to see that deprecated in February too. And again in April. But whats important is cool shiny, right? Not dull corporate commercial "functionality"

@CHEWX
Copy link

CHEWX commented Jun 20, 2022

Any movement here?

@RickMeasham
Copy link

Further Hackery

⚠️ This issue really needs to be properly fixed. While we can disable certain variations, we can't disable the generic embed without losing all variations.


To disable all but YouTube and Vimeo variations in the menu use @gherrink 's solution.

Then to disable transformation-by-paste use the following hackery.

  1. Find all the URL patterns for our allowed embed variations
  2. Replace the isMatch function in the raw transform with one that also checks these collected patterns

Note that this is fragile but as robust as I could make it. If the core/embed changes to storing the variations somewhere other than settings.variations or moves/renames the patterns this will either stop working or break.

const allowedEmbedVariants = [
  'youtube',
  'vimeo',
];

wp.hooks.addFilter(
  'blocks.registerBlockType',
  'saverplus/disable-embed-paste',
  (settings, name) => {
    if (name !== 'core/embed') {
      return settings;
    }

    // Get the URL patterns for our allowed embed variations
    const patterns = [];
    settings?.variations?.forEach(function (variant) {
      if(allowedEmbedVariants.includes(variant.name)) {
          patterns.push(...variant?.patterns);
      }
    });

    // Replace the isMatch function in the raw transform with one that also checks the patterns
    settings.transforms.from[0].isMatch = (node) =>
      node.nodeName === 'P' &&
      /^\s*(https?:\/\/\S+)\s*$/i.test( node.textContent ) &&
      patterns.some( pattern => pattern.test(node.textContent) ) &&
      node.textContent?.match(/https/gi)?.length === 1;

    return settings;
  }
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests