Skip to content

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

Merged
dereksmart merged 2 commits intomasterfrom
fix/stats-deprecated-create_function
Nov 27, 2017
Merged

Stats: Remove usage of create_function() in stats_convert_image_urls()#8240
dereksmart merged 2 commits intomasterfrom
fix/stats-deprecated-create_function

Conversation

@oskosk
Copy link
Copy Markdown
Contributor

@oskosk oskosk commented Nov 24, 2017

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 This PR is ready for review. and removed [Status] In Progress labels Nov 24, 2017
@oskosk oskosk requested review from jeherve and zinigor November 24, 2017 13:49
Copy link
Copy Markdown
Contributor

@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.

return $html;
}

function stats_convert_chart_urls_preg_replace_callback( $matches ) {
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

@oskosk oskosk Nov 27, 2017

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
Copy Markdown
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

Copy link
Copy Markdown
Contributor

@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 This PR is ready for review. label Nov 27, 2017
jeherve added a commit that referenced this pull request Nov 28, 2017
oskosk pushed a commit that referenced this pull request Nov 28, 2017
* 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

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

Development

Successfully merging this pull request may close these issues.

4 participants