Make WordPress Core

Opened 16 months ago

Closed 9 months ago

#53856 closed defect (bug) (fixed)

Check if the 'mirroring' property exists in media-views.js

Reported by: mmdeveloper's profile MMDeveloper Owned by: joedolson's profile joedolson
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch commit
Focuses: javascript, administration Cc:

Description

/wp-includes/js/media-views.js

<?php
this.collection.mirroring.args.s 

on line 2121 should be

<?php
                if ( 
    this.collection.hasOwnProperty('mirroring') &&
    this.collection.mirroring.args.s
){

Similar issue
/wp-includes/js/media-views.min.js

Attachments (1)

53856.3.diff (615 bytes) - added by joedolson 9 months ago.
this.collection.mirroring instead of hasOwnProperty

Download all attachments as: .zip

Change History (20)

#1 @mukesh27
16 months ago

#53857 was marked as a duplicate.

This ticket was mentioned in PR #1537 on WordPress/wordpress-develop by mukeshpanchal27.


16 months ago
#2

  • Keywords has-patch added

#3 @mukesh27
16 months ago

  • Component changed from General to Media
  • Focuses coding-standards removed
  • Version 5.8 deleted

Hi there!

@MMDeveloper thanks for the ticket.

media-views.js and media-views.min.js is automated build files.

PR added as per your suggestion.

#4 @SergeyBiryukov
16 months ago

  • Summary changed from Small Bug to Check if the 'mirroring' property exists in media-views.js

#5 @johnbillion
16 months ago

  • Keywords reporter-feedback added

@mukesh27 @MMDeveloper How can this bug be reproduced please?

#6 @MMDeveloper
16 months ago

I think its to do with legacy software. I'm on WordPress 5.8 but the theme has not been updated in many years. So you may not necessarily care. On WordPress Visual editor, I click edit gallery and when I see WordPress Media Gallery, it produces this error in the Google Developer Tools console.

I think it could be a legacy issue, but considering its just checking if a variable exists or not, I didn't think it was a big deal for you guys to implement in next build.

#7 @SergeyBiryukov
16 months ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 5.9

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


13 months ago

#9 @antpb
13 months ago

  • Owner set to joedolson
  • Status changed from new to assigned

#10 @sabernhardt
13 months ago

  • Milestone changed from 5.9 to 6.0

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


13 months ago

#12 @antpb
13 months ago

  • Milestone changed from 6.0 to 5.9

Joe mentioned this was a good candidate to make it back into the 5.9 milestone so we'll be moving it back in. :D Thanks, Joe!

#13 @hellofromTonya
12 months ago

  • Keywords needs-testing-info needs-testing added
  • Milestone changed from 5.9 to 6.0

5.9 Beta 1 is happening in less than 30 minutes. I'm sorry this ticket didn't get a review and testing in time. As it's a bug that was not introduced in 5.9 cycle, moving it to 6.0 to give it the time and attention it deserves.

Also updated the keywords as testing instructions are needed to reproduce the issue and then verify the patch resolves it.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


10 months ago

#15 @joedolson
10 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 52671:

Comments: Verify 'mirroring' property exists in media views.

Checks that the media collection has a mirroring property before accessing the property in media views.

Props MMDeveloper, mukesh27.
Fixes #53856.

#16 @SergeyBiryukov
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Is there a reason not to just check if ( this.collection.mirroring && ... ) here?

Seems more consistent with seven similar checks in models/attachments.js, and I think it doesn't matter in this case if the property is directly assigned or inherited, just that it exists.

#17 @joedolson
10 months ago

Hmm. I didn't question the original patch on that; I doubt there's any particular reason that direct vs inheritance would matter, but I can test, in case.

@joedolson
9 months ago

this.collection.mirroring instead of hasOwnProperty

#18 @joedolson
9 months ago

  • Keywords commit added; needs-testing-info needs-testing removed

#19 @joedolson
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 52806:

Media: Use consistent mirroring check in media views.

Change the mirroring property verification method to be consistent with other usages across models and attachments, since we don't care whether it's assigned directly on inherited.

Props MMDeveloper, SergeyBiryukov.
Fixes #53856.

Note: See TracTickets for help on using tickets.