Bypass latest Theme Check update for add_submenu_page calls. #460
Conversation
Bypass latest Theme Check update for add_submenu_page calls.
|
Cue evil laugh... |
|
And I have just changed theme-check to once again detect this. Stop trying to bypass theme check, or I'll simply add in a check that bans this library from the Theme directory entirely. This is not acceptable behavior. The call you're trying to make is not allowed in WordPress.org themes, and if you continue down this path, then you're going to get every single theme that uses your library removed from the directory. |
|
The thing is, @Otto42, this library isn't just for themes - it's for plugins too, so forcing our dependency plugins page into the Appearance menu just don't make sense for consuming plugins which use it. The hack we're applying here isn't for our benefit - it's for themes and plugins (in and out of WPORG repos) the flexibility to have our page show up where it makes sense for them and their users. If you can suggest a way to achieve this, that happens to work with the Theme Check Plugin, then we're definitely open to suggestions. |
|
The way that you can achieve it is to pull the functionality that allows the addition of arbitrary admin menus out of the library. As long as this is in the library, theme check will attempt to detect it, and it will cause a failure message. This is intentional. Themes in the directory explicitly are not allowed to do this. Having code in there do something themes are not allowed to do is a fail condition. Simple. This isn't even hard for you to do. I made a fork that does this right here. It passes theme check just fine: If I have to keep changing theme check to bypass your bypasses, then my very next change will be to detect the strings "TGM-Plugin-Activation" and "tgmpa" and cause an instant failure. |
|
I understand the WPORG theme directory rule regarding where custom pages initialised by the theme can be added, and I support that rule. However, you appear to have missed my point entirely. This library is also for plugins, and for themes that are not in the WPORG theme repo. Pulling out that flexibility, without some concession for those theme and plugin authors just isn't feasible. Maybe we can apply a filter or action hook around or near the
Come on Otto - don't resort to pointless threats - you're a far better man than that. You know that doing that would cause the TRT far more hassle than it would for Juliette, Thomas and me. This isn't a commercial venture, so having one group of users not be allowed to use it makes little odds to us. I'd rather we all play nicely together, look at the bigger picture, tap into your experience and come up with a solution that satisfies everyone's requirements. |
|
Look, what I know is that it is extremely annoying to me that I've had to adjust theme check twice specifically because of this library, and if it's going to be a hassle to me, then I will take whatever action I feel is necessary. I have banned things from the site before. I will do it again if required. You're explicitly, and intentionally, changing your code to break the rules of the directory. What action do you expect us to take? For use of the library in plugins, perhaps you should offer them a different library entirely. Or maybe something like a config option where the plugin has to specify the function name to use, so that your library doesn't contain code which straight up violates the rules of the theme directory. Because your primary user is themes, so maybe you should consider that? All I know is you're causing me and the other reviewers headaches. We have to tell theme authors how to modify your code to be compliant, constantly. It's annoying. |
We did - it's called the If we changed that to be some value that took the function name then (with a fallback to |
|
I don't care what other themes or plugins do. What I care about is making theme-check check against the theme review guidelines. The guidelines say that themes can only add menus under appearance. So I check to see if themes try to add menus anywhere else. Simple. Now, your library tries to do exactly that. Whether it is under a config option or not is irrelevant. I can't check your config option. I can't check the config options of every library out there. I'm not running your code, I'm parsing it. So all I can see is that you have a function call that is not allowed. If you don't have that function call, then you don't have any problems. If other people modify it to have that function call, then that's on them and their theme submission would fail. So, fine by me. |
Clearly, but we do. Leave it with us - we won't do any more sneaky hacks to bypass this, and we'll see what we can come up with that is more elegant, ideally backwards-compatible, that everyone is happy with. |
|
Good. I was this close to banning call_user_func() and call_user_func_array() as well as detecting and disallowing arbitrary code execution through variable function names. I don't like having to tighten the rules because of people being sneaky about their coding techniques. It makes code unelegant and harder to read and understand. Annoying. Clean code is best. |
Instead of specifying the parent slug, let's be more specific about the actual function that should be called. By default, we stick with the Appearance menu, as before, with `add_theme_page`. But now, any themes or plugins that want to locate our dependencies plugins page elsewhere has to explicitly pass in a config string of `add_submenu_page` or `add_foo_page()`. This would be outside our library, making the library itself fine for use in WPORG themes, but the customisation can be caught by the Theme Check plugin. See #460, hat-tip @Otto42.
|
@Otto42 I understand your annoyance, however I find your aggressive tone of voice disrespectful and threatening and unacceptable between professional developers. I kindly request you to take a step back, take a deep breath before starting discussions in the way you did here. Having said that, let's look at the actual issue. There are two wp.org theme review rules in play here:
TGMPA however - as @GaryJones pointed out - is used far more widely than just by wp.org repo themes. Maintaining a number of forks would make the project unwieldy and more difficult to maintain and is - in my opinion - not a realistic suggestion nor a realistic request. The responsibility to follow the wp.org rules lies with the theme authors. Putting the responsibility for theme authors to follow the rules on us is turning things upside down. TGMPA very consciously tries to encourage theme authors to follow rule 1 by: As long as theme authors don't change the default, Theme Check appears however to only look at rule 1 and to disregard rule 2. All it would take for Theme Check to respect both rules, would be to check if the If you like, I'll gladly send in a PR to Theme Check (let me know where I can find the repo please) with the code for that and we can close the discussion and remove the hacks within TGMPA. |
Umm, no. There is no such rule in the TRT guidelines. Themes can easily recommend and offer plugin install links without using TGMPA. It's like a few lines of code to do so. |
|
Gary, d5b513b seems okay to me from a quick glance at the changes. |
Thanks. We'll go through and test for BC and other issues, and proceed along those lines for getting a release out. |
|
@kprovance I think that's uncalled for here. Please delete your comment. |
|
Yeah, sure. Don't fear the reaper, bud. |
|
Any updates on when this might get merged and released? Waiting on the update to allow me to upload a .org theme. |
|
@galengidman See above discussion. It won't be. To upload your theme to .org, just remove the logic and replace with a straight add_theme_page() call. |
I'm starting to see "issues" in theme reviews reporting this again. So making our previous bypass-hack less detectable ;-)