Add Check For Theme Activation Constant #950

Closed
wants to merge 10 commits into
from

Projects

None yet

8 participants

@willthemoor

Allows a developer to skip Roots' Theme Activation step by setting a constant:

define('ROOTS_ACTIVATION', FALSE);

From discussion at http://discourse.roots.io/t/disable-activation-page-when-switching-themes/917/

@swalkinshaw
Member

Looks great! Sorry to be picky but can you remove the surrounding spaces in the conditional?

@willthemoor

Whitespace fixed.

Couldn't figure out how to squash the two commits and push up to github so sorry for this history noise. Do I need to close and do a new pull request or does the auto added comment above with the fixed commit represent the same?

@retlehs
Member
retlehs commented Dec 30, 2013

try git rebase -i HEAD~2 and choose to squash them - if you can't figure it out, no worries we can just merge

@willthemoor

that's exactly what I did but when finished I got this:

# On branch theme-activation
# Your branch and 'origin/theme-activation' have diverged,
# and have 1 and 2 different commits each, respectively.

can't push...

$ git push origin theme-activation
To https://github.com/willthemoor/roots
 ! [rejected]        theme-activation -> theme-activation (non-fast-forward)
error: failed to push some refs to 'https://github.com/willthemoor/roots'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

and it seems to me that pull/merge/push adds similar history noise. Happy to do it though.

@swalkinshaw
Member

When you rebase you need to force push with -f

On Sunday, December 29, 2013, Will Moore wrote:

that's exactly what I did but when I'm finished I got this:

On branch theme-activation

Your branch and 'origin/theme-activation' have diverged,

and have 1 and 2 different commits each, respectively.

can't push...

$ git push origin theme-activation
To https://github.com/willthemoor/roots
! [rejected] theme-activation -> theme-activation (non-fast-forward)
error: failed to push some refs to 'https://github.com/willthemoor/roots'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

and it seems to me that pull/merge/push adds similar history noise. Happy
to do it though.


Reply to this email directly or view it on GitHubhttps://github.com/roots/roots/pull/950#issuecomment-31329340
.

@willthemoor

Cheers for the git lesson @swalkinshaw. :) I'll give it another go.

@willthemoor

Looks like that did the trick. Thanks!

@retlehs
Member
retlehs commented Dec 30, 2013

@Foxaii brought up something in our chat earlier this evening.. @willthemoor is there any reason why we couldn't add another add_theme_support that's toggled in lib/config.php to enable/disable activation, rather than using a constant in wp-config.php?

@willthemoor

@retlehs Yes and no. Doing it via add_theme_support is a great idea and feels much cleaner as it keeps roots concerns within roots. But it also means that someone spinning up multiple sites with roots can't set a constant outside of roots (which would presumably be pulled in via git/composer). I'm in middle of rebuilding my WP boilerplate project so I may be overly concerned with that use case. Happy to let it go.

I can see three ways, you can prolly see more!

1) Affirmative

add_theme_support('allow-activation'); 
  if (current_theme_supports('allow-activation')) {
    wp_redirect(admin_url('themes.php?page=theme_activation_options'));
    exit;
  }

2) Negative

add_theme_support('disallow-activation'); 
  if (!current_theme_supports('disallow-activation')) {
    wp_redirect(admin_url('themes.php?page=theme_activation_options'));
    exit;
  }

Negative, I believe, has the benefit of not needing to be present at all in lib/config.php. Putting a negative into add_theme_support() just feels weird though. But...

3) Negative + Constant

Perhaps too messy but might something like this work? Where like is in italics because I've been staring at a screen too long to know if this logic would even work. :)

  if (!current_theme_supports('disallow-activation') || (defined('ROOTS_ACTIVATION') && ROOTS_ACTIVATION)) {
    wp_redirect(admin_url('themes.php?page=theme_activation_options'));
    exit;
  }

I'd appreciate something like 3) for the use case mentioned above but I might have edge case on my forehead.

@Foxaii
Member
Foxaii commented Dec 30, 2013

I would keep the affirmative case, but change how you add theme support to honour the constant if set. Can you test this:

add_theme_support('roots-activation', (defined('ROOTS_ACTIVATION') ? ROOTS_ACTIVATION : true )); // Enable activation options

I would also name the support feature the same as the constant, but that's more of a personal preference.

@willthemoor

@Foxali That looks great. I'll give it a shot.

@ajmalafif

what's the status of the Add Check For Theme Activation Constant feature?

Will it be merged for the next release?

leoj3n and others added some commits Dec 20, 2013
@leoj3n @willthemoor leoj3n Fix gallery link option
Galleries should support three link types:

* Attachment Page (default): [gallery ids="1,2,3"]
* Media File: [gallery link="file" ids="1,2,3"]
* None: [gallery link="none" ids="1,2,3"]
a8eadbb
@willthemoor willthemoor Add Check For Theme Activation Constant
Allows a developer to skip Roots' Theme Activation step by setting a constant:

define('ROOTS_ACTIVATION', FALSE);
8d52542
@retlehs @willthemoor retlehs Remove changing media folder from theme activation 3827134
@retlehs @willthemoor retlehs screen_icon has been deprecated with WP 3.8 c1815c9
@retlehs @willthemoor retlehs Add notice to theme activation, tidy activation table markup 4dd31ab
@retlehs @willthemoor retlehs Generate new roots.pot 73a8b47
@retlehs @willthemoor retlehs Add analytics 36f02b7
@retlehs @willthemoor retlehs Ignore LESS source map cfd32e6
@roscius @willthemoor roscius Match wp_register_script function signature
Although the third parameter currently works when false to indicate
no dependancies, it works by side-effect, the function signature uses
an empty array to indicate no dependancies.
1118bec
@retlehs @willthemoor retlehs Remove analytics 0dbd226
@willthemoor

Well, that's neat. #gitfail. Think I'll close this, fork, add the three lines and submit a new pull request.

@willthemoor willthemoor closed this Jan 9, 2014
@willthemoor

Well, just as well. I ran my test incorrectly anyway. :)

@Foxaii

add_theme_support('roots-activation', (defined('ROOTS_ACTIVATION') ? ROOTS_ACTIVATION : true )); 

doesn't work. It always returns true (even if I set the final true to false manually.

Other ideas?

@willthemoor willthemoor reopened this Jan 9, 2014
@Foxaii
Member
Foxaii commented Jan 9, 2014

Did you try:

define("ROOTS_ACTIVATION", false);

Edit:
You're correct. It looks like the "current_theme_supports" argument isn't being passed. I'll look into this over the weekend.

@Foxaii
Member
Foxaii commented Jan 19, 2014

A weekend late...

One fix would be to directly reference the global variable $_wp_theme_features but it's seems too hacky for my liking. The other option is something like:

if (is_admin() && isset($_GET['activated']) && 'themes.php' == $GLOBALS['pagenow']) {
  if (defined('ROOTS_ACTIVATION') && ROOTS_ACTIVATION == false) { 
    return remove_theme_support('roots-activation'); 
  } elseif (current_theme_supports('roots-activation' || defined('ROOTS_ACTIVATION') && ROOTS_ACTIVATION)) {
    wp_redirect(admin_url('themes.php?page=theme_activation_options'));
    exit;
  }
}

Could you give it a try please @willthemoor?

Edit: Updated example thanks to @swalkinshaw

@QWp6t
Member
QWp6t commented Jan 19, 2014

How come it can't be done through an event?

add_filter('roots-activation', [...]);

[...]

apply_filters('roots-activation', [...]);

Then to disable...

add_filter('roots-activation', '__return_false');

Or if you want to use actions, we could rely on remove_action() to disable activation.

Are there any pitfalls to this approach?

@Foxaii
Member
Foxaii commented Jan 19, 2014

There are lots of different ways to do it but it should be in keeping with the other functions (i.e. enabled through add_theme_support in lib/config.php) and if you can force it off, you should be able to force it on (meaning the same kind of logic would need to be applied, no matter how the feature is structured).

@retlehs retlehs added the feature label Feb 5, 2014
@retlehs retlehs added this to the 7.0.0 milestone Feb 22, 2014
@retlehs retlehs referenced this pull request Feb 22, 2014
Merged

Roots 7.0.0 #982

11 of 13 tasks complete
@retlehs retlehs removed this from the 7.0.0 milestone Jun 30, 2014
@swalkinshaw
Member

@willthemoor if you're interested in this could do a new or cleaned up PR with just the activation changes? We'll give you a bit to respond but if not we'll probably do a version of this ourselves as @Foxaii last suggested.

@willthemoor

Sorry for falling off the map. @Foxaii's way looks good to me.

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