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

chore: update CKEditor build to v4.14.1 #1402

Merged
merged 1 commit into from
Jul 1, 2020
Merged

chore: update CKEditor build to v4.14.1 #1402

merged 1 commit into from
Jul 1, 2020

Conversation

julien
Copy link
Contributor

@julien julien commented Jun 29, 2020

Due to: https://nvd.nist.gov/vuln/detail/CVE-2020-9281
(Previous attempt in #1396)

Closes: #1395

@wincent
Copy link
Contributor

wincent commented Jun 30, 2020

So, to help me review this, what is different in this compared to the original attempt (other than the obvious v4.14.0 to v4.14.1 change)? And how did you test it to know that it works?

@wincent
Copy link
Contributor

wincent commented Jun 30, 2020

I sent #1405 to salvage the bit of #1396 that is still relevant.

@julien
Copy link
Contributor Author

julien commented Jul 1, 2020

There's nothing different from the previous attempt apart that this is using v4.14.1 instead of v4.14.0 of CKEditor.

To test it, I ran yarn build and yarn test (various times)

I also ran yarn start and checked the demo, edited the "demo page" by adding tables, making text bold, italic, added links, etc...

I then tested this "local" build inside my fork of liferay-portal - in the Blogs, Web Context and Site builder sections and everything is working as expected.

@wincent
Copy link
Contributor

wincent commented Jul 1, 2020

That sounds good enough to me!

@wincent
Copy link
Contributor

wincent commented Jul 1, 2020

Seeing as we still have that problem with yarn test not actually running all the tests, I also tested this using yarn test:debug in Firefox (because Chrome hangs).

Master:

Screenshot 2020-07-01 -123545-Zz3nsuu9@2x

This PR:

Screenshot 2020-07-01 -123834-tXsNxcnt@2x

So FAILED count actually goes down by 1 on this PR, but PASSED goes down by 2; that's a very crude indication though because for the "PASSED" count I'm just counting lines that don't have the word "FAILED" in them 😬, and the log does print out other things...

Master:

Screenshot 2020-07-01 -124329-kfN5WaF4@2x

Screenshot 2020-07-01 -124351-kF1DEY32@2x

Second run had the above errors, plus these:

Screenshot 2020-07-01 -124454-v4j8J3DF@2x

Screenshot 2020-07-01 -124523-XLGiWxdp@2x

That first one seems dependent on window size (seems to appear when I make the console too big).

This PR:

Screenshot 2020-07-01 -124110-fek0BsxK@2x

In short, a total shit-show. PR is unlikely to make things worse, may make them slightly better, so let's merge.

@wincent wincent merged commit ca322b6 into liferay:master Jul 1, 2020
@julien
Copy link
Contributor Author

julien commented Jul 1, 2020

I forgot about yarn test:debug, I'll give it a shot anyway to see if I can spot anything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update bundled CKEditor version
2 participants