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

Makes cleanForSlug more robust when receiving falsy values. #16236

Merged

Conversation

AVGP
Copy link
Contributor

@AVGP AVGP commented Jun 20, 2019

Description

Fixes https://core.trac.wordpress.org/ticket/47216

Currently, cleanForSlug is used a few times with the assumption that it'll return a falsy value for a falsy input (see post-link or post-permalink) but it crashes as it assumes the argument to be string.

How has this been tested?

I ran this change in a local WordPress instance with the testing procedure outlined in the WordPress issue.

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 22, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

How about we add a simple unit test which covers this case?

Thank you for your contribution, by the way 👍

packages/editor/src/utils/url.js Outdated Show resolved Hide resolved
@gziolo gziolo added [Package] Editor /packages/editor [Type] Bug An existing feature does not function as intended labels Jun 22, 2019
@AVGP
Copy link
Contributor Author

AVGP commented Jun 22, 2019

Added a unit test as well 🚀

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

The proposed code looks good. I haven't tested it with the use case described in Trac ticket so I can't confirm it fixes the issue it tries to mitigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Editor /packages/editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants