WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years 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:
PR Number:

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 3 years ago.
37514.diff (1.5 KB) - added by obenland 3 years ago.
37514.2.patch (2.2 KB) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (18)

@ocean90
3 years ago

#1 @obenland
3 years 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
3 years ago

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

@obenland
3 years ago

#3 @obenland
3 years ago

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

#4 @jorbin
3 years ago

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

#5 @swissspidy
3 years ago

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

#6 @ocean90
3 years 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
3 years 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.


3 years ago

#9 follow-up: @swissspidy
3 years 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
3 years 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
3 years 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
3 years ago

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

#13 @ocean90
3 years 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
3 years ago

#14 @jorbin
3 years 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
3 years 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.