Conversation
| target[ part ] = Object.create( null ); | ||
| // If target[part] is not an object or array, initialize as empty object. | ||
| if ( typeof target[ part ] !== 'object' || target[ part ] === null ) { | ||
| target[ part ] = {}; |
There was a problem hiding this comment.
Why such a change? This was changed recently to avoid prototype pollution.
There was a problem hiding this comment.
Because it cause error in mathtype manual test (inside wiris mathtype lib).
On objects created this way (Object.create( null )) there is no hasOwnProperty method so if we create language ui by using define mathtype throws error when trying to read language because they use hasOwnProperty.
There was a problem hiding this comment.
I agree with @niegowski comment. This part of the code is responsible for preventing prototype pollution, which is also flagged by code scanners. The change is fully intentional. Bypassing security measures just to apply a bugfix isn’t a good practice. It’s better to fix this on the side of the library that relies on this behavior.
| // We take care of proper config structure. | ||
| if ( !isPlainObject( target[ name ] ) ) { | ||
| target[ name ] = Object.create( null ); | ||
| target[ name ] = {}; |
There was a problem hiding this comment.
Why such a change? This was changed recently to avoid prototype pollution.
| /** | ||
| * Function _translate from translation-service.ts gets translations from | ||
| * global.window.CKEDITOR_TRANSLATIONS when translations injected into Locale are empty. | ||
| * Value of global.window.CKEDITOR_TRANSLATIONS is provided by CKEditorTranslationsPlugin from | ||
| * ckeditor5-dev-translations package (for example in manual tests) when --language flag is used. | ||
| * Function _translate is called often and has no access to the editing editor config, so here is a better place to | ||
| * check if translations will be taken from dev-translations, and if yes – update config.language.ui value. | ||
| */ |
There was a problem hiding this comment.
imho, this comment could be clearer. Two things:
- It sounds like
global.window.CKEDITOR_TRANSLATIONSis always set byCKEditorTranslationsPlugin, but that’s not true. It can also come from translation files loaded from a CDN, so we shouldn’t assume it always comes from the plugin. - Without reading the issue (#7297), it’s hard to tell what the problem is and why this
ifis needed. The comment should explain it in a more straightforward way.
|
i added pr to wiris to resolve mathtype manual error wiris/html-integrations#1107 |
|
There has been no activity on this PR for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the contribution, leave a comment or reaction under this PR. |
4 participants
Suggested merge commit message (convention)
Tests: Fix manual tests language.ui value.
Additional information
Closes https://github.com/cksource/ckeditor5-commercial/issues/7297.