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.

Sign up for GitHub

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

Jump to bottom

Late escape Navigation blocks #37870

Merged
merged 18 commits into from
Jan 14, 2022
Merged

Late escape Navigation blocks #37870

merged 18 commits into from
Jan 14, 2022

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Jan 11, 2022

Description

This is not a security problem. This PR simply moves escaping of all PHP output to be as "late" as possible. This means we avoid escaping variables until they are output in the HTML markup.

This is a WP Core best practice.

How has this been tested?

  • add block
  • check functionality is "as was"
  • check all tests pass

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@getdave getdave added [Type] Code Quality Issues or PRs that relate to code quality [Block] Navigation Affects the Navigation Block [Type] Security Related to security concerns or efforts [Block] Navigation Link Affects the Navigation Link Block [Block] Submenu Affects the Submenu Block - for submenus in navigation labels Jan 11, 2022
@getdave getdave self-assigned this Jan 11, 2022
@getdave getdave changed the title Try/late escape navigation blocks Late escape Navigation blocks Jan 11, 2022
@getdave
Copy link
Contributor Author

getdave commented Jan 13, 2022

Ok this PR seems to have a few issues. Let me revisit with a more critical eye and iron out some of these.

@getdave
Copy link
Contributor Author

getdave commented Jan 13, 2022

Ok @swissspidy and @tellthemachines this is RfR again. I appreciate your patience with this PR. In all honesty I could have handled it with greater care and attention. That's my fault for trying to take on too many blocks at the same time.

I've made the necessary changes and we now have a limited subset of changes which should be easier to review.

tellthemachines
tellthemachines reviewed Jan 14, 2022
View reviewed changes
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for the iteration, this is looking much better now!

I'm not sure the test failures are legit, so restarted the build, let's see how that goes.

packages/block-library/src/navigation-submenu/index.php Show resolved Hide resolved
@getdave
Copy link
Contributor Author

getdave commented Jan 14, 2022

Ok I think were now seeking a 👍 on this one.

swissspidy
swissspidy approved these changes Jan 14, 2022
View reviewed changes
Copy link
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

LGTM with one question about escaping inline styles

@getdave getdave merged commit 30e57e1 into trunk Jan 14, 2022
@getdave getdave deleted the try/late-escape-navigation-blocks branch January 14, 2022 14:35
@github-actions github-actions bot added this to the Gutenberg 12.5 milestone Jan 14, 2022
@getdave getdave mentioned this pull request Feb 7, 2022
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgefilipecosta jorgefilipecosta jorgefilipecosta left review comments

@swissspidy swissspidy swissspidy approved these changes

@talldan talldan Awaiting requested review from talldan

@ehti ehti Awaiting requested review from ehti

@adamziel adamziel Awaiting requested review from adamziel

@draganescu draganescu Awaiting requested review from draganescu

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

Assignees

@getdave getdave

Labels
[Block] Navigation Link Affects the Navigation Link Block [Block] Navigation Affects the Navigation Block [Block] Submenu Affects the Submenu Block - for submenus in navigation [Type] Code Quality Issues or PRs that relate to code quality [Type] Security Related to security concerns or efforts
Projects
None yet
Milestone
Gutenberg 12.5
Development

Successfully merging this pull request may close these issues.
4 participants
@getdave @swissspidy @tellthemachines @jorgefilipecosta