Skip to content

Sitemaps: Make the reporter log success messages only if JETPACK_DEV_DEBUG is defined #8184

Merged
oskosk merged 1 commit intomasterfrom
remove/sitemaps-error_log
Nov 22, 2017
Merged

Sitemaps: Make the reporter log success messages only if JETPACK_DEV_DEBUG is defined #8184
oskosk merged 1 commit intomasterfrom
remove/sitemaps-error_log

Conversation

@oskosk
Copy link
Copy Markdown
Contributor

@oskosk oskosk commented Nov 16, 2017 •
edited
Loading

Makes the report method Jetpack_Sitemap_Logger ::report() only log success messages if JETPACK_DEV_DEBUG is defined.

Error logs in several sites I'm using are filled with this piece of information which is not actually useful, specially when WP_DEBUG is turned on to observe possible errors happening. Message are like:

jp-sitemap-fDyFX: -- Building sitemap-1.xml
jp-sitemap-WTjbS: 0.123 seconds elapsed.
jp-sitemap-fDyFX: -- Cleaning Up jp_sitemap
jp-sitemap-fDyFX: -- Cleaning Up jp_sitemap_index

Changes proposed in this Pull Request:

  • Updates the check for WP_DEBUG to be a check for both WP_DEBUG and JETPACK_DEV_DEBUG instead for success messages and just a check for WP_DEBUG for error messages.

Testing instructions:

  • Activate sitemaps
  • Define WP_DEBUG to true.
  • Rebuild the sitemaps (wp jetpack sitemaps rebuild).
  • Define JETPACK_DEV_DEBUG to true.
  • Expect to see the success messages only when JETPACK_DEV_DEBUG is defined.

@oskosk oskosk requested a review from a team as a code owner November 16, 2017 10:04
@oskosk oskosk changed the title Make sitemaps log only if JETPACK_DEV_DEBUG is defined Sitemaps: Make the reporter log only if JETPACK_DEV_DEBUG is defined Nov 16, 2017
@oskosk oskosk added the [Status] Needs Review This PR is ready for review. label Nov 16, 2017
@oskosk
Copy link
Copy Markdown
Contributor Author

oskosk commented Nov 16, 2017

@eliorivero would you say this is fine ? (I think you authored this line and the original check for WP_DEBUG)

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Nov 16, 2017

I would personally vote to keep those when WP_DEBUG is on, as it can be useful when debugging Sitemaps issues on live sites, where we would not want to turn on Jetpack Dev mode. Just my personal opinion though.

@oskosk
Copy link
Copy Markdown
Contributor Author

oskosk commented Nov 16, 2017 •
edited
Loading

I was going to reply the following:
But what is the useful information here provided by these messages for the general audience with WP_DEBUG enabled and expecting to see only errors there?

... But you have a point. Still, I would remove the success messages that clutter the log with, IMO meaningless information about the success. Why log that everything went well for just this particular feature of Jetpack ?

@oskosk
Copy link
Copy Markdown
Contributor Author

oskosk commented Nov 16, 2017

What if we log the success messages if both WP_DEBUG and JETPACK_DEV_DEBUG are defined and the errors if WP_DEBUG is defined ?

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Nov 16, 2017

What if we log the success messages if both WP_DEBUG and JETPACK_DEV_DEBUG are defined and the errors if WP_DEBUG is defined ?

I like that. That makes perfect sense to me. 👍

@oskosk oskosk force-pushed the remove/sitemaps-error_log branch from 6190f71 to fe6d86e Compare November 16, 2017 22:01
@oskosk oskosk changed the title Sitemaps: Make the reporter log only if JETPACK_DEV_DEBUG is defined Sitemaps: Make the reporter log success messages only if JETPACK_DEV_DEBUG is defined Nov 16, 2017
@oskosk
Copy link
Copy Markdown
Contributor Author

oskosk commented Nov 16, 2017

Thanks for the feedback @jeherve !
I've just updated this PR to reflect the proposal. Title and description and testing instructions updated too.

If this just does not help in anything for debugging, we can close it.

jeherve
jeherve approved these changes Nov 22, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Nov 22, 2017
@oskosk oskosk merged commit 9ab0f89 into master Nov 22, 2017
@oskosk oskosk deleted the remove/sitemaps-error_log branch November 22, 2017 14:37
@oskosk oskosk removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 22, 2017
@oskosk oskosk added this to the 5.6 milestone Nov 22, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
@jeherve
Changelog: add #8184
ebcb1ee
oskosk pushed a commit that referenced this pull request Nov 28, 2017
@jeherve @oskosk
Changelog 5.6: running changelog. (#8192)
7844644 
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

@zinigor zinigor Awaiting requested review from zinigor

@dereksmart dereksmart Awaiting requested review from dereksmart

Assignees

No one assigned

Labels

[Feature] Sitemaps [Type] Janitorial

Projects

None yet

Milestone

5.6

Development

Successfully merging this pull request may close these issues.

2 participants

@oskosk @jeherve