Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stats: Remove usage of create_function() in stats_convert_image_urls() #8240

Merged
merged 2 commits into from
Nov 27, 2017
Merged

Stats: Remove usage of create_function() in stats_convert_image_urls() #8240

merged 2 commits into from
Nov 27, 2017

Conversation

oskosk
Copy link
Contributor

@oskosk oskosk commented Nov 24, 2017 •
edited
Loading

Fixes #8010

Changes proposed in this Pull Request:

  • Replaces a function created with create_function for passing as callback to preg_replace_callback. A new function called stats_convert_chart_urls_preg_replace_callback which does the same is introduced.

Testing instructions:

  • Start with a site running on PHP 7.2 with WP_DEBUG (and maybe WP_DEBUG_LOG too) (/specialops, can help)
  • Connect Jetpack
  • Check this branch and visit Jetpack's admin page dashboard
  • Expect to see not logged message about create_function being deprecated.

@oskosk oskosk added [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Status] In Progress [Type] Janitorial labels Nov 24, 2017
@oskosk oskosk requested a review from a team as a code owner November 24, 2017 13:40
@oskosk oskosk added this to the 5.6 milestone Nov 24, 2017
@oskosk oskosk added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Nov 24, 2017
@oskosk oskosk requested review from zinigor and jeherve November 24, 2017 13:49
zinigor
zinigor previously requested changes Nov 27, 2017
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Looks good overall, but there's one thing that I'd love us to improve here.

modules/stats.php Outdated
@@ -654,6 +654,11 @@ function stats_convert_image_urls( $html ) {
return $html;
}

function stats_convert_chart_urls_preg_replace_callback( $matches ) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's namespace this with jetpack_ please. In order to make things easier, we can remove the _replace_callback in the end and instead specify that this is going to be used as a callback inside a docblock.

Copy link
Contributor Author

@oskosk oskosk Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Alright! I guess I tried to name the function consistently with the way all of the functions are named without a jetpack_ prefix here. Will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in adfad17

@oskosk oskosk dismissed zinigor’s stale review November 27, 2017 16:35

Requested changes have been added

dereksmart
dereksmart approved these changes Nov 27, 2017
View reviewed changes
Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

LGTM

@dereksmart dereksmart merged commit f933eae into master Nov 27, 2017
@dereksmart dereksmart deleted the fix/stats-deprecated-create_function branch November 27, 2017 18:25
@dereksmart dereksmart removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 27, 2017
jeherve added a commit that referenced this pull request Nov 28, 2017
@jeherve
Changelog: add #8240
5e18376
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

@dereksmart dereksmart dereksmart approved these changes

@zinigor zinigor zinigor left review comments

@jeherve jeherve Awaiting requested review from jeherve

Assignees
No one assigned
Labels
[Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Type] Janitorial
Projects
None yet
Milestone
5.6
Development

Successfully merging this pull request may close these issues.
4 participants
@oskosk @zinigor @dereksmart @jeherve