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

variables in template files are flagged as global #200

Open
joyously opened this issue Feb 12, 2019 · 14 comments

Comments

@joyously
Copy link

commented Feb 12, 2019

The theme template files are supposed to be loaded with core functions. When they are, the variables used in them are not global, so do not need to be prefixed.

Each line with a variable is generating an error for Overriding WordPress globals is prohibited and Variables defined by a theme/plugin should start with the theme/plugin prefix.

Do we need a whitelist or keep track of which files are used with include or requre?

@jrfnl

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

The theme template files are supposed to be loaded with core functions.

That's the crux of it: "supposed to". However, there is no guarantee that that's the only way that these files will be loading - think: plugins doing funky stuff -, so it's always better to be safe and prefix the variables, than sorry.

@joyously

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

If it's going to be flagged, maybe Error is too severe.

@justintadlock

This comment has been minimized.

Copy link

commented Feb 18, 2019

This is a problem. If there's no way of detecting whether the variable is global or skipping templates, I'd say we need to disable this for the time being. Otherwise, theme authors will have to whitelist or prefix every single variable used in a template.

Even a warning, I believe, will be a hindrance to the TRT's review process because it'd mean additional time checking false-positives. But, I could live with that as a middle ground.

Variables defined by a theme/plugin should start with the theme/plugin prefix.

This notice is also not worded correctly. The guideline related to this is more appropriately:

Global variables defined by a theme/plugin should start with the theme/plugin prefix.

@jrfnl

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Just to be clear: the variables we are talking about here are declared as global variables based on the file alone.
It's only by the happenstance that the file is included from within a function call in some other file, that they become local to that function.

This notice is also not worded correctly

I concur this can do with improvement, but that's something for WPCS.

@justintadlock

This comment has been minimized.

Copy link

commented Feb 18, 2019

We can nitpick over whether the feature is happenstance or intentional, but that doesn't really matter. Personally, I intentionally use this PHP feature to do some pretty cool stuff, like passing data to templates (though I don't generally assign variables in a template).

What matters is that the notice is incorrect by the TRT guidelines. The team neither recommends nor requires that such variables be prefixed. Therefore, I'd recommend we disable it for now.

@jrfnl

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

We can nitpick over whether the feature is happenstance or intentional, but that doesn't really matter

That's not nitpicking, that's basic understanding of the principles of how PHP works.

What matters is that the notice is incorrect by the TRT guidelines. The team neither recommends nor requires that such variables be prefixed. Therefore, I'd recommend we disable it for now.

  • The sniff was added based on a rule in the TRT guidelines - see the discussion in #129
  • The sniff is not active by default. It only becomes active when a prefix is passed to the PHPCS run.
  • The sniff itself should not be deactivated/disabled as it checks for the prefixing of all global constructs, not just variables.
    If anything, only the errorcode regarding the prefixing of global variables should be deactivated/disabled.
  • If so desired though, a better solution would be to extend the WPCS sniff, overload the method which deals with prefixing variables and bowing out early if a theme template file is detected.
    That way, a file like functions.php would still be examined for global variables being properly prefixed, while template files would not be.
    How to detect whether a file is a theme template file, is something which would need to be discussed. I imagine a whitelist of file names / file name patterns might work, but other ideas are welcome.

I'd like to suggest for this issue to be discussed in a TRT meeting, bearing the above input in mind.

@justintadlock

This comment has been minimized.

Copy link

commented Feb 19, 2019

That's not nitpicking, that's basic understanding of the principles of how PHP works.

Which was my point. It simply works like this because that's how PHP works.

The sniff is not active by default. It only becomes active when a prefix is passed to the PHPCS run.

Then, this should be a non-issue as far as the TRT is concerned. I was given the impression that it was a default check. If it is not now nor is it planned to become one of the default checks, it's not a high priority.

The sniff itself should not be deactivated/disabled as it checks for the prefixing of all global constructs, not just variables.
If anything, only the errorcode regarding the prefixing of global variables should be deactivated/disabled.

That's what I was asking for--that we disable the prefixing of global variables part.

@joyously

This comment has been minimized.

Copy link
Author

commented Feb 19, 2019

The sniff is not active by default. It only becomes active when a prefix is passed to the PHPCS run.

Using it with a prefix is the best way to use it for themes, and that means it gives errors for all the variables in a template file.

@dingo-d

This comment has been minimized.

Copy link
Member

commented May 18, 2019

A sniff for this has been merged and will be in the next release (0.2.0). I'll close this to clean up the issues a bit.

@dingo-d dingo-d closed this May 18, 2019

@jrfnl

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

@dingo-d The fix which was merged addresses the PrefixAllGlobals part of the issue only.
Should this issue stay open to further discuss whether or not to address the GlobalVariablesOverride part of it as well ?

@dingo-d

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Oh, I haven't taken that into the account. Reopening and adding the necessary label 🙂

@joyously

This comment has been minimized.

Copy link
Author

commented Jun 3, 2019

I think the WP global variables should still be caught in template files, because that's how the template files work (all the query vars are declared as global where the template is loaded, so that the globals are available). Those global names should not be assigned to in the template file, just like anywhere else, and should get flagged GlobalVariablesOverride.
Using set_query_var to pass data to a template is fine, but the variable has to be prefixed, since it is becomes global.
So what's left are incidental variables used in the template file.

@dingo-d

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Ok, the GlobalVariablesOverride sniff will work in the templates (we didn't touch that), so I guess we're good on that front.

So what's left are incidental variables used in the template file.

What would be an example of an incidental variable?

@joyously

This comment has been minimized.

Copy link
Author

commented Jun 4, 2019

I don't see how you would distinguish between a variable that was set using set_query_var, which becomes global and needs a prefix, and other variables that are used as temporary (as if in a function, because that's the context) and are not global and don't need a prefix.
I suppose if it's a template file, it could be a Warning, and if not, it could be an Error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.