JSON.parse error breaks WYSIWYG when embedded JSON is invalid [#2873141]

When the embed JSON is edited manually and is done invalid by mistake, whole CKEditor is broken after the form submission which makes it hardly possibly to fix the JSON.

Comment	File	Size	Author
#2	video_embed_field-wysiwyg-handle-invalid-json-2873141-2-D8.patch	684 bytes	Alex Bukach
#2	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 Skipped, branch did not pass for 4 weeks PHP 5.5 & SQLite 3.8, D8.4 111 pass, 1 fail PHP 5.6 & MySQL 5.5, D8.4 111 pass, 1 fail PHP 7 & MySQL 5.5, D8.4 111 pass, 1 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

26 April 2017 at 20:22

Alex Bukach created an issue. See original summary.

Comment #2

Alex Bukach CreditAttribution: Alex Bukach commented 26 April 2017 at 20:25

Assigned:	Alex Bukach	» Unassigned
Status:	Active	» Needs review

File	Size
video_embed_field-wysiwyg-handle-invalid-json-2873141-2-D8.patch	684 bytes
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 Skipped, branch did not pass for 4 weeks PHP 5.5 & SQLite 3.8, D8.4 111 pass, 1 fail PHP 5.6 & MySQL 5.5, D8.4 111 pass, 1 fail PHP 7 & MySQL 5.5, D8.4 111 pass, 1 fail

We should gracefully exit when the JSON is invalid.

Comment #3

26 April 2017 at 20:29

Status:

Needs review

» Needs work

The last submitted patch, 2: video_embed_field-wysiwyg-handle-invalid-json-2873141-2-D8.patch, failed testing.

Comment #4

Alex Bukach CreditAttribution: Alex Bukach commented 26 April 2017 at 21:17

The fails looks having no relation to the patch at all. Shouldn't one re-run the tests for the module itself against the latest version of Drupal?

Comment #5

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented 26 April 2017 at 22:31

That does indeed look like a fail in HEAD.

I'm not sure about silently failing. At least when it errors out you have an indication of what is wrong. Perhaps we should throw an error after catching the JSON parsing error describing in more detail how to fix the problem?

Comment #6

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented 14 August 2020 at 01:50

Status:

Needs work

» Closed (won't fix)

With the advent of Media in core, the Video Embed Field module has moved to being minimally maintained. Only issues which assist in the migration to Media in core will be committed. To read more about this decision, please see: #3089599: Maintenance status for Video Embed Field now that media is in core.