WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 17 months ago

Last modified 17 months ago

#37514 closed defect (bug) (fixed)

Shiny Updates: Remove console.error() calls

Reported by: ocean90 Owned by: jorbin
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Upgrade/Install Keywords: has-patch commit
Focuses: javascript, administration Cc:

Description

There are a few window.console.error() calls which happen when a bulk action is used which isn't handled by shiny updates. Maybe I'm missing something, but I don't see a need for them.

Attachments (3)

37514.patch (669 bytes) - added by ocean90 17 months ago.
37514.diff (1.5 KB) - added by obenland 17 months ago.
37514.2.patch (2.2 KB) - added by ocean90 17 months ago.

Download all attachments as: .zip

Change History (18)

@ocean90
17 months ago

#1 @obenland
17 months ago

I added them for more transparency around unexpected uses of these functions. They could help developers working with update.js to extend it correctly, and others to mention them in bug reports if they run into them. I know they've been helpful for me during development of SUv2.

How about we account for the core bulk actions in the switch statement and bail instead of removing them?

#2 @ocean90
17 months ago

@obenland Can you work on a patch for that variant? We should also check if console.error exists before using it.

@obenland
17 months ago

#3 @obenland
17 months ago

Patch attached. In a future release: Make the defaults filterable :)

#4 @jorbin
17 months ago

If we are expecting these strings to help developers, shouldn't they be translatable?

#5 @swissspidy
17 months ago

FWIW we removed those calls in the decrementUpdateCount method after replacing the switch statement with if clauses.

#6 @ocean90
17 months ago

Is it really an error if the bulk action hander in updates.js gets triggered for a custom action? This sounds more like a warning/info message to me.

#7 @obenland
17 months ago

If we are expecting these strings to help developers, shouldn't they be translatable?

Not sure how making the strings available for translation would fit in with the soft string freeze.

FWIW we removed those calls in the decrementUpdateCount method after replacing the switch statement with if clauses.

I'm not married to doing this, if you all think it's not worth keeping feel free to pull them out :)

Is it really an error if the bulk action hander in updates.js gets triggered for a custom action? This sounds more like a warning/info message to me.

Sure, that would work too

This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.


17 months ago

#9 follow-up: @swissspidy
17 months ago

Yeah, window.console.warning could work too, but then we should remove the return; statements.

Let's say we're going to add bulk actions to wp-admin/update-core.php in Shiny Updates V3 and want to re-use some of the existing code in core. With errors and and return; statements everywhere that's not easily possible without completely overriding the JS (which is hard to maintain).

See [37971] for the change I mentioned. A simple if block would look cleaner than a few empty case blocks.

#10 in reply to: ↑ 9 ; follow-up: @obenland
17 months ago

Replying to swissspidy:

Yeah, window.console.warning could work too, but then we should remove the return; statements.

Removing the return statement for bulk action wouldn't work since the default behavior of the form submission will get prevented. Let's just remove them? Seems to be the most plausible solution right now.

#11 in reply to: ↑ 10 @ocean90
17 months ago

  • Keywords commit added
  • Owner set to jorbin
  • Status changed from new to reviewing

Replying to obenland:
Let's just remove them? Seems to be the most plausible solution right now.

+1, see 37514.patch. @jorbin, can you give this a review?

#12 @jorbin
17 months ago

There is still a console.log on line 254 after the patch is applied. Should we also clean that up here?

#13 @ocean90
17 months ago

@jorbin That one used for debugging. But is should probably get a window.console check.

if ( 'undefined' !== typeof response.debug && window.console && window.console.log ) { … }

@ocean90
17 months ago

#14 @jorbin
17 months ago

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

In 38186:

Updates: Clean up debug statements.

Removing some console.error calls leftover from development and wrapping the console.log call in a check to ensure console.log exists.

Fixes #37514.
Props ocean90, obenland

#15 @jorbin
17 months ago

In 38188:

Updates: Clean up debug statements.

Merge of [38186] to the 4.6 branch

Removing some console.error calls leftover from development and wrapping the console.log call in a check to ensure console.log exists.

Fixes #37514.
Props ocean90, obenland

Note: See TracTickets for help on using tickets.