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

Don't re-encode already encoded post titles #2407

Merged
merged 3 commits into from
Aug 21, 2017

Conversation

notnownikki
Copy link
Member

Data that comes from the Posts collection is already encoded, ready to output safely, so we set the inner html to avoid encoding twice.

Fixes #2389

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be best for us to avoid dangerouslySetInnerHTML if at all possible if alternatives exist (hence its name).

In this case, there are other options available for unescaping entities:

@notnownikki
Copy link
Member Author

@aduth lodash doesn't unescape everything we need, and I agree that he is too big... I'll try creating a textarea to use for decoding, and reuse the element so we're not doing element creation for every post.

@codecov
Copy link

codecov bot commented Aug 15, 2017

Codecov Report

Merging #2407 into master will increase coverage by 0.59%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2407      +/-   ##
==========================================
+ Coverage    25.9%   26.49%   +0.59%     
==========================================
  Files         157      161       +4     
  Lines        4853     5170     +317     
  Branches      822      903      +81     
==========================================
+ Hits         1257     1370     +113     
- Misses       3035     3180     +145     
- Partials      561      620      +59
Impacted Files Coverage Δ
blocks/library/latest-posts/index.js 10% <ø> (ø) ⬆️
utils/entities.js 90% <90%> (ø)
blocks/library/code/index.js 40% <0%> (-10%) ⬇️
blocks/library/paragraph/index.js 39.28% <0%> (-7.78%) ⬇️
blocks/library/preformatted/index.js 37.5% <0%> (-6.95%) ⬇️
blocks/library/table/index.js 30% <0%> (-6.37%) ⬇️
blocks/library/quote/index.js 13.11% <0%> (-1.18%) ⬇️
blocks/library/heading/index.js 22.85% <0%> (-0.96%) ⬇️
blocks/editable/index.js 10.36% <0%> (-0.61%) ⬇️
blocks/library/list/index.js 6.55% <0%> (-0.42%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77620b2...7433aca. Read the comment docs.

this.decodeEntities = this.decodeEntities.bind( this );
}

decodeEntities( html ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this might be a utility worth generalizing? Maybe moving to utils/decode-entities.js and including some unit tests? There may even be some overlap between this and what's being proposed by @adamsilverstein as an @wordpress/sanitize utility at WordPress/packages#12 (which takes it a step further to strip HTML tags).

While it wouldn't be wise to generalize before there's a need, we've already seen some use of Lodash's _.unescape that could be better served by something like this:

renderCategoryName( category ) {
if ( ! category.name ) {
return __( '(Untitled)' );
}
return unescape( category.name ).trim();
}

And if Calypso's usage can serve as evidence, it's a very common utility:

https://github.com/Automattic/wp-calypso/search?utf8=%E2%9C%93&q=decodeentities&type=

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... ok, happy to generalize this :)

@notnownikki
Copy link
Member Author

Added an 'entities' module that has 'decodeEntities'.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

You may consider reaching out on WordPress/packages#12 to see if we can reconcile requirements here, since I think the new utility falls well under what's being proposed there.

@notnownikki notnownikki merged commit f08c5f4 into master Aug 21, 2017
@notnownikki notnownikki deleted the fix/htmlentities-in-latest-posts branch August 21, 2017 09:36
ceyhun pushed a commit that referenced this pull request Jun 23, 2020
…hing in landscape mode (#2407)

* fix columnsInRow on init and some performance improvements

* update ref: rename getColumnWidth to contentStyle

* update ref to master
ceyhun added a commit that referenced this pull request Jun 23, 2020
* Update gutenberg submodule ref

* Re-enable list spaces test

* Update gutenberg submodule ref

* Display radial gradient in block which supporting gradients (#2266)

* Update Aztec to version 1.19.2

* Commit hermes locally so we can build gutenberg using jitpack without installing all dependencies

* Remove hermes headers

* Update gutenberg ref before merge

* yarn version to 1.29.0

* Fix release note formatting

* Update to official version of Aztec

* Merge pull request #2322 from wordpress-mobile/issue/update_aztec_to_1.19.2

Update to official version of Aztec 1.19.2

* Update podfile.lock file.

* Fix link insertion at start of lists.

* Update github actions/cache to v2

* Only run gallery e2e tests

* Update version to 1.28.2

* Update bundles.

* Try different jest config

* Add 1.28.2 release note

* Try different jest config

* Add logs

* Update GB reference.

* Revert "Add logs"

This reverts commit 823d9bd.

* Add log

* Revert "Add log"

This reverts commit f684dc3.

* Add package-lock.json changes after npm install

* Force exit jest after all tests have completed running

* Update bundles

* Update podfile.lock

* Remove Keyboard.dismiss when deleting block (#2327)

* Update ref

* Update ref to point to master

* Update GB reference.

* Adjust how we're oulling theme elements from the bundle

* Update GB reference.

* Update bundles.

* Update list spaces test to not be canary test

* Update gutenberg submodule ref

* Update bundles

* Update bundle

* Fix: Button crash (#2332)

* Update GB reference.

* Update gb reference

* Update bundle files

* Update gutenberg ref

* Resolve Podfile.lock conflict

* Update .gitignore

* Update bridge build

* Update podfile.

* Fix link error in react-native-bridge/package.json

* Fix react-native entry in @wordpress/react-native-bridge package.json

* Update gutenberg ref

* Remove unused commands and update clean commands

* Add RNGetRandomValuesPackage to WPAndroid glue code

* Update working directory for npm related bridge commands

* Add contributing file.

* Decrease sleep time for android recording process

* Disable npm cache for android and ios runners

* Re-enable npm cache but use exact key for restore

* Use underscores with code of conduct file

* Update package-lock.json by re-adding react-native-editor to bust npm cache

* Remove nvm step from iOS runner

* Update bundle

* Decrease sleep time for android recording process

* Ignore and log android screen recording errors

* Update GB reference.

* Remove unused imports

* Update release notes and update gutenberg reference

* Recreate bundle

* Remove unnecessary patch files

* Try setting max worker to 2 to avoid error 137 on circle CI

* Update GB reference.

* update ref

* Update version to 1.29.1

* Update gutenberg submodule ref

* Update GB reference.

* Update bundles.

* Allow unsupported blocks to be edited on mobile (#2063)

Unsupported blocks can now be edited through a web view which utilizes the Gutenberg web editor. This is only available on development builds

Co-authored-by: Eduardo Toledo <eduardo.toledo@automattic.com>
Co-authored-by: Marko Savic <savicmarko1985@gmail.com>
Co-authored-by: Paul Von Schrottky <paul.von.schrottky@automattic.com>

* Update GB reference.

* Update GB reference.

* Update GB reference.

* Sort the release notes in the correct order.

* Update GB reference.

* Add release checklist

* Update GB reference.

* Update the VS code search settings to that seaching is easier

* Update version number

* Update Aztec-iOS instructions to reference ReleaseProcess.md file

wordpress-mobile/AztecEditor-iOS#1294

* Update bundles

* Added reference to release checklist template to template

* Remove info about bumping the version when not releasing

* Update release notes.

* Setup handshake mechanism for gutenberg bridge

* Simplify handshake

* Updated commands in README.md

* Adjust call for replaying events to call super to avoid the custom logic on the instance

* Revert unneeded changes and remove mocked event

* Update GB reference.

* Update bundles.

* Bump the version of the gutenberg that contains the fix

* Update to the latest version of the gutenberg submodule.

* Update gutenberg regerence

* Update bundle

* Update bridges methods to receive HTML content information metrics.

* Rename content info variables.

* Link wordcount package.

* Update to exclude the whole `bundle` folder

* Only exclude the App.js and App.js.map build files.

* Use Kotlin in react-native-gutenberg-bridge

* Use Xcode version 11.4.1 when running e2e tests

* Update package-lock.json

* Android implementation of the bridge.

* Add queue for deferred events

* Split and rename media upload message interfaces

* Implement a deferred event emitter

* Update release notes.

* Show all available information.

* Fix type gutenber => gutenberg

* Remove the gutenberg core build files since they should be ignored by default.

* Display block count information.

* Update GB reference.

* Adjust threading and method for queue

* Revert gutenberg change

* Trigger getting content and info

* Update RELEASE-NOTES

* Send non-critical messages safely from deferred event emitter

* Bump gutenberg version

* Bump gutenberg version

* Rerun bundle

* Refactor decoding step to an extension for ContentInfo

* Update format for content info.

* Move Gutenberg WebView related files to rn-gutenberg-bridge folder

* Update Android assets symlink to new assets folder

* Update GB reference.

* Adjust notification name to use existing extensions

* Update GB reference.

* Update GB reference.

* Update GB reference.

* Update bundles to support content structure on HTML request.

* Rename yarn_install to npm_install

* Radial gradient infrastructure (#2277)

* Fix react-native-bridge podspec

* Update to the latest gutenberg

* Simplify react-native-editor sass-transformer

* Fix react-native-bridge import

* Delete already deleted Media.java from react-native-bridge

* Fix lint erros

* Adjust optionality

* Fix duplicate kotlin_module error

* Rename interfaces in demo app

* Add release note

* Update package-lock.json

* The RN dep lives in the devDependencies section

* The gb-mobile root is now two more folders app

since the bridge code got two folders inside the gutenberg/packages
folder.

* Fix gutenberg-web-single-block symlink for unsupported blocks

* react-native-recyclerview is not used anymore

* Remove react-native-editor/bin folder

* Remove mentions of yarn, build is via npm now

* Use npm ci for reproducible builds

* [Fix] Columns block renders more than two columns in a row when launching in landscape mode (#2407)

* fix columnsInRow on init and some performance improvements

* update ref: rename getColumnWidth to contentStyle

* update ref to master

* Revert "Use npm ci for reproducible builds"

This reverts commit 6f7b9e5.

* Use latest node lts and npm versions

* Update react-native-bridge gradle script's path to root

Also added a check to insure that this kind of mistake gets caught
quickly in the future since otherwise it only reveals itself at runtime.

* Update bundleUpToDateCheck task to handle monorepo changes

* Check .scss files for changes

* Ignore changes to bundle directory and its subdirectories when doing up-to-date check

* Only check package.json for dependency changes

* Fix formatting

* Remove feature branch trigger from Travis CI

* Resolve merge issue in design doc

* Resolve merge issue in link-control/index.js

* Resolve merge issue in navigation-link/editor.scss

* Remove renamed angle-picker folder

* Update react-native-aztec README

* Remove react-native-aztec LICENSE file in favor of the repo's own license

* Update react-native-bridge README

* Delete GitHub issue and PR templates from react-native-editor

* Remove react-native-editor CONTRIBUTING file in favor of the repo's own one

* Remove react-native-editor LICENSE file in favor of the repo's own license

* Update react-native-editor README

* Remove top-level react-native script

* Update babel-jest to version 25.3.0

* Print message during execution instead of configuration

* Give more descriptive name to mobile gutenberg path variable

* Update packages/react-native-editor/README.md

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

* Update packages/react-native-editor/README.md

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

* Update .editorconfig to use spaces in files for Android

* Delete both node_modules folder when cleaning

* Extract file object construction out of loop

* Declare node modules folders at single location

* Add .npmrc files in react-native-* packages

Co-authored-by: Matt Chowning <matt.chowning@automattic.com>
Co-authored-by: Luke Walczak <lukasz.walczak.pwr@gmail.com>
Co-authored-by: Sergio Estevao <sergioestevao@gmail.com>
Co-authored-by: Ceyhun Ozugur <ceyhunozugur@gmail.com>
Co-authored-by: Chip Snyder <chip.snyder3@gmail.com>
Co-authored-by: Chip <chip.snyder@automattic.com>
Co-authored-by: Dratwas <drapich.piotr@gmail.com>
Co-authored-by: etoledom <etoledom@icloud.com>
Co-authored-by: Eduardo Toledo <eduardo.toledo@automattic.com>
Co-authored-by: Marko Savic <savicmarko1985@gmail.com>
Co-authored-by: Paul Von Schrottky <paul.von.schrottky@automattic.com>
Co-authored-by: Enej Bajgoric <enej.bajgoric@automattic.com>
Co-authored-by: Cameron Voell <cameronvoell@gmail.com>
Co-authored-by: Matthew Kevins <mmkevins@yahoo.com>
Co-authored-by: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Co-authored-by: Matthew Kevins <mkevins@users.noreply.github.com>
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
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.

2 participants