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.

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

[ON HOLD] Move date function group from RestrictedPHPFunctions to TimezoneChange #1794

Closed
wants to merge 1 commit into from

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Sep 11, 2019

Follow up on PR #1714 which fixed #1713.

As per the discussion in #1731 (comment). moving the check for use of the date() to the TimezoneChange sniff.

As the date function group has not been in a released version (yet), we can still safely make this change without concerns about introducing breaking changes (error code change).

As that sniff is not part of the Core ruleset, I'm explicitly adding this particular error code to the Core ruleset.

Note: we may want to consider renaming this sniff as the name does not seem to be 100% correct anymore.

Suggestions for a better name welcome. If we go down that road, I'd deprecate the TimezoneChange sniff to be removed in the next major release.

@jrfnl jrfnl added Type: Chores/Cleanup Status: Review ready Component: Core Focus: WP interoperability Doing things the WP way, prefer using WP functionality over userland/PHP native labels Sep 11, 2019
@jrfnl jrfnl added this to the 2.2.0 milestone Sep 11, 2019
@jrfnl jrfnl force-pushed the feature/move-function-check-date branch 4 times, most recently from 128194c to 3337752 Compare September 12, 2019 04:26
…Change`

Follow up on PR 1714 which fixed 1713.

As per the discussion in 1731#discussion_r301840793. moving the check for use of the `date()` to the `TimezoneChange` sniff.

As the `date` function group has not been in a released version (yet), we can still safely make this change without concerns about introducing breaking changes (error code change).

As that sniff is not part of the `Core` ruleset, I'm explicitly adding this particular error code to the `Core` ruleset.

Note: we may want to consider renaming this sniff as the name does not seem to be 100% correct anymore.

Suggestions for a better name welcome. If we go down that road, I'd deprecate the `TimezoneChange` sniff to be removed in the next major release.
@jrfnl
Copy link
Member Author

jrfnl commented Sep 12, 2019

⚠️ PLEASE DO NOT MERGE THIS (YET) ⚠️

This PR is on hold until a decision has been taken as outlined in #1791 (comment)

@jrfnl jrfnl changed the title Move date function group from RestrictedPHPFunctions to TimezoneChange [ON HOLD] Move date function group from RestrictedPHPFunctions to TimezoneChange Sep 12, 2019
@jrfnl jrfnl added Focus: DateTime and removed Focus: WP interoperability Doing things the WP way, prefer using WP functionality over userland/PHP native labels Oct 4, 2019
@jrfnl
Copy link
Member Author

jrfnl commented Oct 5, 2019

Closing in favour of PR #1807 which addresses the proposal as put forward in #1805.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discourage date() in favor of gmdate()
1 participant