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

New category: DateTime (WPCS 2.2.0) #1805

Closed
jrfnl opened this issue Oct 3, 2019 · 6 comments · Fixed by #1807
Closed

New category: DateTime (WPCS 2.2.0) #1805

jrfnl opened this issue Oct 3, 2019 · 6 comments · Fixed by #1807

Comments

@jrfnl
Copy link
Member

jrfnl commented Oct 3, 2019

Is your feature request related to a problem?

Currently, the only DateTime related sniff in a released version of WPCS is the WP.TimezoneChange sniff.

PR #1714 added a check for use of the date() function to the PHP.RestrictedPHPFunctions sniff in response to issue #1713 and is targetted to be included in WPCS 2.2.0.
There is a blocked PR #1794 which tentatively moves this check to the TimeZoneChange sniff.

And then there is issue #1791 which is requesting a sniff to check for the use of current_time() in combination with certain parameter values.

Loosely related:

Describe the solution you'd like

In #1791 (comment) I'm basically already outlining what I have in mind. I'm opening this issue now to get a decision on it.

I'd like to propose the following:

  • Add a new category DateTime.
    Within this category have:
    • A RestrictedFunctions sniff.
      The check for use of the date_default_timezone_set() function would be moved from the WP.TimezoneChange sniff to this new sniff.
      The check for use of the date() function would be moved from the PHP.RestrictedPHPFunctions sniff to this new sniff.
    • A CurrentTime sniff which would address issue Discourage use current_time( 'timestamp' ) #1791.
  • Deprecate the WP.TimezoneChange sniff in WPCS 2.2.0 in favour of the DateTime.RestrictedFunctions sniff and remove it in WPCS 3.0.0.
  • Add the complete DateTime category to the Core ruleset.

Having a separate category makes it so these type of checks don't get scattered around and is in line with prior art, like the WPCS DB category.

It also makes it easier to add/remove all DateTime related checks from a custom ruleset in one go by just referencing the category instead of the individual sniffs/error codes.

Timing-wise, making this change now seems opportune as only the WP.TimezoneChange is currently available in a released version of WPCS.

Also, as discussed in #1654, once the PHPCSUtils repo launches, work will start on WPCS 3.0.0 which would then become the next version, which means that the deprecated sniff won't have to remain in the codebase for very long.

Opinions ?

/cc @Rarst

@jrfnl jrfnl added this to the 2.2.0 milestone Oct 3, 2019
@Rarst
Copy link
Contributor

Rarst commented Oct 4, 2019

A dedicated category does make sense, given it is a dedicated core component. There would likely more things to go there, especially if we get to formal deprecations of old stuff in future.

@GaryJones
Copy link
Member

+1 from me.

@dingo-d
Copy link
Member

dingo-d commented Oct 27, 2019

This is closed by accident or?

@jrfnl
Copy link
Member Author

jrfnl commented Oct 27, 2019

@dingo-d Not really. The category has now been added and the one sniff which needed moving has moved.

New sniffs, like the DateTime.CurrentTime sniff as pulled in #1808 can now easily be added to the category.

Am I forgetting something ?

@dingo-d
Copy link
Member

dingo-d commented Oct 27, 2019

Oh I thought that issues like these are closed only when a new release is made, my mistake 🙂

@jrfnl
Copy link
Member Author

jrfnl commented Oct 27, 2019

No worries ;-)

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

Successfully merging a pull request may close this issue.

4 participants