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

Remove CollegeHumor from embed providers #18591

Merged
merged 16 commits into from Nov 22, 2019

Conversation

@audrasjb
Copy link
Contributor

audrasjb commented Nov 18, 2019

CollegeHumor doesn't support oEmbed anymore, so this PR removes the related blocks.

For reference, see the related Trac ticket: https://core.trac.wordpress.org/ticket/48696
Fixes #18567

Cheers,
Jb

Copy link
Member

gziolo left a comment

Thanks @audrasjb for starting this PR. I'm afraid we can't simply remove this embed because this will create backward compatibility issues for posts created in the past. Users would see the message like:
Your site doesn’t include support for the "core-embed/collegehumor" block...

There is some prior art though where @ice9js added a deprecation logic for the Crowdsignal embed. See #12854. I think it can be replicated here, the difference is that we don't need to add a new embed but probably change the pattern for the YouTube embed.

@audrasjb

This comment has been minimized.

Copy link
Contributor Author

audrasjb commented Nov 21, 2019

Thanks for the review @gziolo !
I updated the patch accordingly :)

Jb Jb
@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 21, 2019

I'm afraid we can't simply remove this embed because this will create backward compatibility issues for posts created in the past. […]

There is some prior art though […]

I came here because of the same thing. This is definitely something we need to fix in the embed blocks framework. Embed blocks probably just need to be multiple expressions of a single block type, so that we don't need to maintain these things ad hoc.

Jb Jb
@audrasjb

This comment has been minimized.

Copy link
Contributor Author

audrasjb commented Nov 21, 2019

Ok! Thanks @gziolo, makes sense.
I updated the patch accordingly.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 21, 2019

I googled to find some old url, example:
http://www.collegehumor.com/video/60123495/real-adults-dont-order-dessert

Screen Shot 2019-11-21 at 12 59 45

When I embed it, it get recognized as a general embed when I paste it. Is that okay?

@audrasjb

This comment has been minimized.

Copy link
Contributor Author

audrasjb commented Nov 21, 2019

@gziolo Looks good to me.
FYI, non embed will be generated at all outside the block editor.
https://core.trac.wordpress.org/attachment/ticket/48696/Capture%20d%E2%80%99%C3%A9cran%202019-11-18%20%C3%A0%2020.47.52.png

@gziolo
gziolo approved these changes Nov 21, 2019
Copy link
Member

gziolo left a comment

We will have to re-test it once the core patch lands.

Let's move forward as is. I applied the label Needs Dev Note to ensure we revisit it later. I assume we will need a note for WordPress 5.4 release for the core part anyway.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 21, 2019

One more thing to unblock merging, https://travis-ci.com/WordPress/gutenberg/jobs/258989943#L608:

 /home/travis/build/WordPress/gutenberg/packages/block-library/src/embed/core-embeds.js
609[0]   148:5  error  Missing trailing comma  comma-dangle
610[0] 
611[0] ✖ 1 problem (1 error, 0 warnings)
612[0]   1 error and 0 warnings potentially fixable with the `--fix` option.

npm run lint-js:fix should be all that is necessary.

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 21, 2019

Following up on #18591 (comment), although this doesn't address the issue of a single block type expressed as many blocks, it does "work" as a quick quick concept of handling missing embeds:

diff --git a/packages/blocks/src/api/parser.js b/packages/blocks/src/api/parser.js
index 6300eac32..4182375a9 100644
--- a/packages/blocks/src/api/parser.js
+++ b/packages/blocks/src/api/parser.js
@@ -455,9 +455,17 @@ export function createBlockWithFallback( blockNode ) {
 			innerHTML = originalContent;
 		}
 
-		name = unregisteredFallbackBlock;
-		attributes = { originalName, originalContent, originalUndelimitedContent };
-		blockType = getBlockType( name );
+		// If detected as an embed whose definition is missing, automatically
+		// convert to generic embed.
+		if ( name.indexOf( 'core-embed/' ) === 0 ) {
+			name = 'core/embed';
+			blockType = getBlockType( name );
+			innerHTML = originalUndelimitedContent;
+		} else {
+			name = unregisteredFallbackBlock;
+			attributes = { originalName, originalContent, originalUndelimitedContent };
+			blockType = getBlockType( name );
+		}
 	}
 
 	// Coerce inner blocks from parsed form to canonical form.
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 21, 2019

@mcsf - did you just propose a way to deprecate all individual embeds and integrate them as one block type with different providers defined as patterns? 😃

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 21, 2019

😆

Jb Jb
@audrasjb

This comment has been minimized.

Copy link
Contributor Author

audrasjb commented Nov 21, 2019

@gziolo I tried npm run lint-js:fix but merging is still blocked, sorry 😬

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 22, 2019

There seem to be some intermittent test failures. Re-running failed tests.

@@ -15864,8 +15864,7 @@
"ansi-regex": {
"version": "2.1.1",
"bundled": true,
"dev": true,
"optional": true
"dev": true

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 22, 2019

Member

There are some unrelated changes included in this file. I bet they should be removed from this patch.

This comment has been minimized.

Copy link
@audrasjb

audrasjb Nov 22, 2019

Author Contributor

This is the result of npm run lint-js:fix command on my branch. Should I remove them with a new commit?

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 22, 2019

Member

It's rather from npm install :)
Yes, please remove it.

This comment has been minimized.

Copy link
@audrasjb

audrasjb Nov 22, 2019

Author Contributor

Thanks @gziolo, we should be good to go now :)

Jb added 2 commits Nov 22, 2019
@gziolo gziolo merged commit 2529722 into WordPress:master Nov 22, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Failed
Details
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 22, 2019

Thanks @audrasjb for this patch 👍

@audrasjb

This comment has been minimized.

Copy link
Contributor Author

audrasjb commented Nov 23, 2019

For your information, the related Core ticket has been committed and reopened for 5.3.1 consideration.
https://core.trac.wordpress.org/ticket/48696

@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
youknowriad added a commit that referenced this pull request Dec 5, 2019
* Remove CollegeHumor from embed provider since the service doesn’t exists anymore.

* Update deprecation logic

* Update deprecated pattterns

* Remove unwanted patterns

* lint fixes

* undo unwanted changes in package-lock.json

* remove unwanted changes, second pass

* Remove CollegeHumor from embed provider since the service doesn’t exists anymore.

* Update deprecation logic

* Update deprecated pattterns

* Remove unwanted patterns

* lint fixes

* undo unwanted changes in package-lock.json

* remove unwanted changes, second pass
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Dec 9, 2019
The list of included fixes:

- Edge scrolling issues WordPress/gutenberg#17707
- Intermittent JavaScript issues WordPress/gutenberg#18559
- Remove embed CollegeHumor WordPress/gutenberg#18591 

Updated packages:

- @wordpress/block-directory@1.0.6
- @wordpress/block-editor@3.2.5
- @wordpress/block-library@2.9.6
- @wordpress/core-data@2.7.5
- @wordpress/edit-post@3.8.6
- @wordpress/editor@9.7.6
- @wordpress/format-library@1.9.5

Props youknowriad, ellatrix, epiqueras, audrasjb, gziolo, mcsf, kyliesabra.
Fixes #48884.

git-svn-id: https://develop.svn.wordpress.org/trunk@46860 602fd350-edb4-49c9-b593-d223f7449a82
nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Dec 9, 2019
The list of included fixes:

- Edge scrolling issues WordPress/gutenberg#17707
- Intermittent JavaScript issues WordPress/gutenberg#18559
- Remove embed CollegeHumor WordPress/gutenberg#18591 

Updated packages:

- @wordpress/block-directory@1.0.6
- @wordpress/block-editor@3.2.5
- @wordpress/block-library@2.9.6
- @wordpress/core-data@2.7.5
- @wordpress/edit-post@3.8.6
- @wordpress/editor@9.7.6
- @wordpress/format-library@1.9.5

Props youknowriad, ellatrix, epiqueras, audrasjb, gziolo, mcsf, kyliesabra.
Fixes #48884.

git-svn-id: https://develop.svn.wordpress.org/trunk@46860 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Dec 9, 2019
The list of included fixes:

- Edge scrolling issues WordPress/gutenberg#17707
- Intermittent JavaScript issues WordPress/gutenberg#18559
- Remove embed CollegeHumor WordPress/gutenberg#18591 

Updated packages:

- @wordpress/block-directory@1.0.6
- @wordpress/block-editor@3.2.5
- @wordpress/block-library@2.9.6
- @wordpress/core-data@2.7.5
- @wordpress/edit-post@3.8.6
- @wordpress/editor@9.7.6
- @wordpress/format-library@1.9.5

Props youknowriad, ellatrix, epiqueras, audrasjb, gziolo, mcsf, kyliesabra.
Fixes #48884.
Built from https://develop.svn.wordpress.org/trunk@46860


git-svn-id: http://core.svn.wordpress.org/trunk@46660 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Dec 9, 2019
The list of included fixes:

- Edge scrolling issues WordPress/gutenberg#17707
- Intermittent JavaScript issues WordPress/gutenberg#18559
- Remove embed CollegeHumor WordPress/gutenberg#18591 

Updated packages:

- @wordpress/block-directory@1.0.6
- @wordpress/block-editor@3.2.5
- @wordpress/block-library@2.9.6
- @wordpress/core-data@2.7.5
- @wordpress/edit-post@3.8.6
- @wordpress/editor@9.7.6
- @wordpress/format-library@1.9.5

Props youknowriad, ellatrix, epiqueras, audrasjb, gziolo, mcsf, kyliesabra.
Fixes #48884.
Built from https://develop.svn.wordpress.org/trunk@46860


git-svn-id: https://core.svn.wordpress.org/trunk@46660 1a063a9b-81f0-0310-95a4-ce76da25c4cd
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Dec 9, 2019
The list of included fixes:

- Edge scrolling issues WordPress/gutenberg#17707
- Intermittent JavaScript issues WordPress/gutenberg#18559
- Remove embed CollegeHumor WordPress/gutenberg#18591 

Updated packages:

- @wordpress/block-directory@1.0.6
- @wordpress/block-editor@3.2.5
- @wordpress/block-library@2.9.6
- @wordpress/core-data@2.7.5
- @wordpress/edit-post@3.8.6
- @wordpress/editor@9.7.6
- @wordpress/format-library@1.9.5

Props youknowriad, ellatrix, epiqueras, audrasjb, gziolo, mcsf, kyliesabra.
Merges [46860] to the 5.3 branch.
Fixes #48884.

git-svn-id: https://develop.svn.wordpress.org/branches/5.3@46861 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Dec 9, 2019
The list of included fixes:

- Edge scrolling issues WordPress/gutenberg#17707
- Intermittent JavaScript issues WordPress/gutenberg#18559
- Remove embed CollegeHumor WordPress/gutenberg#18591 

Updated packages:

- @wordpress/block-directory@1.0.6
- @wordpress/block-editor@3.2.5
- @wordpress/block-library@2.9.6
- @wordpress/core-data@2.7.5
- @wordpress/edit-post@3.8.6
- @wordpress/editor@9.7.6
- @wordpress/format-library@1.9.5

Props youknowriad, ellatrix, epiqueras, audrasjb, gziolo, mcsf, kyliesabra.
Merges [46860] to the 5.3 branch.
Fixes #48884.
Built from https://develop.svn.wordpress.org/branches/5.3@46861


git-svn-id: http://core.svn.wordpress.org/branches/5.3@46661 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Dec 9, 2019
The list of included fixes:

- Edge scrolling issues WordPress/gutenberg#17707
- Intermittent JavaScript issues WordPress/gutenberg#18559
- Remove embed CollegeHumor WordPress/gutenberg#18591 

Updated packages:

- @wordpress/block-directory@1.0.6
- @wordpress/block-editor@3.2.5
- @wordpress/block-library@2.9.6
- @wordpress/core-data@2.7.5
- @wordpress/edit-post@3.8.6
- @wordpress/editor@9.7.6
- @wordpress/format-library@1.9.5

Props youknowriad, ellatrix, epiqueras, audrasjb, gziolo, mcsf, kyliesabra.
Merges [46860] to the 5.3 branch.
Fixes #48884.
Built from https://develop.svn.wordpress.org/branches/5.3@46861


git-svn-id: https://core.svn.wordpress.org/branches/5.3@46661 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.