Block Parser: Disable console.info in production #33153
Conversation
The block validation will display a console.info for blocks succesfully updated during the validation process. This PR wraps that console.info in a check for production so it is not displayed for produciton instances.
|
Size Change: +619 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
We have always logged these validation notices, intentionally, both on production and dev builds. They can be important clues when user content is automatically migrated/updated, especially in the context of troubleshooting, and they are useful to extenders and admins even when running production.
I don't think it should be on the table to suppress these without some discussion and without first asking some questions. What has changed, why now? Is the volume of notices greater? If so, what does that mean? What is being validated: just user content, or perhaps some application fixtures (e.g. block examples or patterns)? I suspect there are lots of fixtures across Gutenberg and themes that need to be updated to the latest shapes of blocks.
If we don't give this due diligence, we'll just normalise validation notices into irrelevance.
Totally agree here. Based on my testing, there were no notices present in 5.7.2 when visiting the default Sample Page in WordPress vs. in 5.8 development releases (beta1-RC1). The code firing this notice has been in place for over 2 years, so the change causing the increase in notices is coming from a different location. I'm not familiar enough with recent internal changes to know what may have changed to cause the number of |
|
These notices are triggered by block patterns. |
Playing devil's advocate, I'd argue that PHP notices and warnings are a similar parallel to these notices. But those are hidden unless I would not be opposed to leaving these notices as visible for any site running the plugin, but thinking it does not benefit the majority of users to display these console notices in Core itself unless debugging has been turned on. |
|
I updated the code with a better implementation, using The previous production check wasn't great because then it wouldn't even show using plugin to test. Also, for future reference there is a warning package that would've been better choice if trying to limit in production. @mcsf @desrosj I see both sides of keeping and removing, so will let y'll decide if it should be merged or not. Also, it looks like the root cause is related to #33017 and @ntsekouras / @Mamaduka fixed in #33117 but not sure why both those would be in RC1 - is it a dependency issue of pulling future block patterns to an older editor? See WordPress/pattern-directory#245 |
|
I've been testing in 2 environments: 1 running 5.7.2 w/ twentytwenty and 1 running 5.8-RC1 w/ a child of twentyseventeen (also happened as early as 5.8-beta1 I think). In 5.7.2 I get no To further test, I switched the 5.7.2 site to use twentyseventeen and the 5.8.X site to use twentytwenty. Same results: no This suggests that something has change din 5.8 about how |
The crucial difference between regular PHP notices and these is that these pertain to user content (while regular notices pertain to execution of code, which is the domain of the admin or developer) and they reflect potential modifications of user content.
I appreciate the follow-up and trying to find a compromise, @mkaz, though I'd still like to keep the logging in place and weed out runaway block validations.
Thanks for digging, that seems like the best lead. It seems the fix landed in the plugin but is yet to be backported to core. @ntsekouras is away for the day, but he may confirm our next move on Monday. |
These are kind of two different things. #33017 is not in core as it was merged in GB #33148 issue is about deprecation warnings coming from block patterns. In 5.8 there have been introduced many enhancements and features around block patterns that require the parsing of them during the loading of the editor (in browser's idle time though to be non-blocking). The parsing is needed and cannot be avoided.
I agree with this 100%! |
|
Thinking a bit more about it, it’s inevitable that with block patterns we will see more and more of those logs. Maybe, we should be less concerned about successful migrations. I see a ton of What worries me here, that those logs come from |
|
#34163, which was just merged, might a good enough solution to close this one. WDYT? |
Description
The block validation will display a console.info for blocks succesfully updated during the validation process. This PR wraps that console.info in a check for production so it is not displayed for produciton instances.
Fixes #33148
How has this been tested?
Open a post created in a previous version (sample post with buttons works)
Confirm validation notice is shown in console (info needs to be checked)
Apply fix and run against production build
npm run buildTest again and confirm no console.info
Types of changes
Wraps console.info with a production environment check.