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

Only check MediaPlaceholder allowedTypes array length if prop exists #11694

Merged
merged 3 commits into from Nov 13, 2018

Conversation

Projects
None yet
3 participants
@kadamwhite
Contributor

kadamwhite commented Nov 9, 2018

Description

Adds an existence check to allowedTypes before attempting to access allowedTypes.length to fix #11692.

How has this been tested?

  • tested this patch against three custom blocks which utilize MediaPlaceholder without allowedTypes and confirmed that in all cases the patch allows these custom blocks to load as they did several weeks ago.
  • introduced a basic unit test to catch the bug described in the issue

Types of changes

Bug fix: Add an undefined/null check to an array before accessing .length.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • N/A My code follows the accessibility standards.
  • My code has proper inline documentation.
Only check MediaPlaceholder allowedTypes array length if allowedTypes…
… prop exists

Fixes #11692

Adds basic unit test to demonstrate the bug described in #11692.
@@ -142,7 +142,7 @@ class MediaPlaceholder extends Component {
let instructions = labels.instructions || '';
let title = labels.title || '';
if ( ! instructions || ! title ) {
const isOneType = 1 === allowedTypes.length;
const isOneType = 1 === allowedTypes && allowedTypes.length;

This comment has been minimized.

@ocean90

ocean90 Nov 10, 2018

Member

You probably wanted to do this:

Suggested change Beta
const isOneType = 1 === allowedTypes && allowedTypes.length;
const isOneType = allowedTypes && 1 === allowedTypes.length;

This comment has been minimized.

@ocean90

ocean90 Nov 10, 2018

Member

What do you think about defining allowedTypes as an empty array by default?

This comment has been minimized.

@kadamwhite

kadamwhite Nov 10, 2018

Contributor

Right you are on the boolean 🤦‍♂️ And I considered initializing allowedTypes to [] in the destructuring assignment, but then backed off because we do a if ( ! this.props.allowedTypes ) check elsewhere. Upon reflection, since the default initialization won't mutate the value of the prop, the two approaches aren't incompatible and I think your suggestion's the way to go. Will push a commit momentarily.

@@ -142,7 +142,7 @@ class MediaPlaceholder extends Component {
let instructions = labels.instructions || '';
let title = labels.title || '';
if ( ! instructions || ! title ) {
const isOneType = 1 === allowedTypes.length;
const isOneType = allowedTypes.length;

This comment has been minimized.

@ocean90

ocean90 Nov 10, 2018

Member

The check needs to stay :)

This comment has been minimized.

@kadamwhite

kadamwhite Nov 11, 2018

Contributor

🤦‍♂️

@kadamwhite

This comment has been minimized.

Contributor

kadamwhite commented Nov 11, 2018

@ocean90 Thanks for the review, apologies for the sloppy commits! Pushed up a fix, should be ready for review again once Travis has its say.

@ocean90 ocean90 added this to the 4.4 milestone Nov 12, 2018

@@ -131,7 +131,7 @@ class MediaPlaceholder extends Component {
onHTMLDrop = noop,
multiple = false,
notices,
allowedTypes,
allowedTypes = [],

This comment has been minimized.

@youknowriad

youknowriad Nov 12, 2018

Contributor

I see that the prop is also used in onlyAllowsImages and onlyAllowsImages methods, should we add the default values there too?

This comment has been minimized.

@kadamwhite

kadamwhite Nov 12, 2018

Contributor

I looked into that earlier, and I don't think changes are necessary in either of those cases. On onlyAllowsImages and onFilesUpload both, the existing logic should handle both [] and null/false/undefined/etc. (onlyAllowsImages has the checks in the media placeholder component, while onFilesUpload passes the prop value through to be checke within the mediaUpload method)

@youknowriad youknowriad merged commit eb4ed63 into master Nov 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/media-placeholder-do-not-crash-on-missing-allowedtypes branch Nov 13, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Only check MediaPlaceholder allowedTypes array length if prop exists (W…
…ordPress#11694)

* Only check MediaPlaceholder allowedTypes array length if allowedTypes prop exists

Fixes WordPress#11692

Adds basic unit test to demonstrate the bug described in WordPress#11692.

* Initialize allowedTypes to [] instead of modifying conditional

* Reinstate array length check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment