There is an option to enable nodejs on some or all pages. But It would be great to have also the opposite: disable nodejs on certain pages.

CommentFileSizeAuthor
#15 add_disable_nodejs_pages-2120805-15.patch2.43 KBsocialnicheguru
#9 add_disable_nodejs_pages-2120805-9.patch2.29 KBjulien66
#7 nodejs-js-check.png67.82 KBAnonymous (not verified)
#6 add_disable_nodejs_pages-2120805-6.patch2.31 KBjulien66
#2 add_disable_nodejs_pages-2120805-2.patch4.06 KBJorge Navarro

Comments

Anonymous’s picture

Anonymous (not verified) commented
Version: 7.x-1.6 » 7.x-1.x-dev

seems like a useful feature to me, patches welcome.

Jorge Navarro’s picture

Jorge Navarro commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new add_disable_nodejs_pages-2120805-2.patch4.06 KB

It works, but note that this is my first patch, advises are welcome.

socialnicheguru’s picture

socialnicheguru commented

how can I check to see if the nodejs is added on an excluded page in order to test?

socialnicheguru’s picture

socialnicheguru commented

I think this works

julien66’s picture

julien66 commented

Easy testing on this topic could be done with nodejs_checker module !
=> https://drupal.org/project/nodejs_checker

I'm going for some test right now and will report back.
Cheers.

julien66’s picture

julien66 commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new add_disable_nodejs_pages-2120805-6.patch2.31 KB

Hey Jorge !

I confirm this patch is working like a charm. Congratulation.
I did some refactoring of your code. It's the same thing, just shorter and hopefully cleaner.

When two functions are looking very similar, it usually worth trying to refactor the code using a variable. You can compare both code to get the idea. Also think about deleting new variables you set by using variable_del() into hook_uninstall.

=> @Beejebus, I set this issue as reviewed & tested. Please set the authorship to Jorge for his first patch !
Cheers.
++

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new nodejs-js-check.png67.82 KB

this looks good. a couple of things i'd like to see changed:

- please take the $disabled out of the function signature, and check that inside the inside the body of nodejs_add_js_to_page_check()
- i wonder if we should update the form to look more like block visibility, see image below

like blocks

Anonymous’s picture

Anonymous (not verified) commented
Status: Reviewed & tested by the community » Needs work
julien66’s picture

julien66 commented
Status: Needs work » Needs review
StatusFileSize
new add_disable_nodejs_pages-2120805-9.patch2.29 KB

Hi Beejeebus.

Here's a new patch that does what's requested at #7.
Cheers !

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

thanks! getting closer :-)

+  $valid_page = drupal_match_path(drupal_get_path_alias(), variable_get('nodejs_pages', ''));

this doesn't look quite right. as i read it, i think that will reverse the current default behaviour. i think we need something like this:

function nodejs_add_js_to_page_check() {
  global $user;
  
  // Default behaviour is add to all pages.
  $valid_page = TRUE;
  $valid_user = TRUE;

  $page_match_type = variable_get('nodejs_pages_match_type', 'whitelist');
  $pages_default = $page_match_type == 'whitelist' ? '*' : '';

  // Only run the regex if this is not the 'allow all' whitelist.
  if ($page_match_type == 'whitelist' && $pages_default != '*') {
    // If the whitelist is empty, skip the regex.
    $valid_page = $pages_default && drupal_match_path(drupal_get_path_alias(), variable_get('nodejs_pages', $pages_default));
  }

  // Only run the regex if we have a non-empty blacklist.
  if ($page_match_type == 'blacklist' && $pages_default) {
    $valid_page = !drupal_match_path(drupal_get_path_alias(), variable_get('nodejs_pages', $pages_default));
  }

  if (variable_get('nodejs_authenticated_users_only', FALSE)) {
    $valid_user = $user->uid > 0;
  } 

  return $valid_page && $valid_user;
}

does that make sense? explicitly name the type of list we are using, and only run the regex if we need to. sorry for being so picky.

julien66’s picture

julien66 commented

Hi Beejeebus,

Thanks for your fast reply and for your eyes.
I had the same doubt than yours writing this line, I wrote almost the same code as you did before I realised it wasn't necessary at all.
The actual behavior of patch #9 is already correct from my point of view. Here's how I see it :
By now, default behavior is to set Nodejs active into => all page except thoses listed, with a blank list '' as variable.
It's the opposite than before where Nodejs was active in => only the listed page, with a wildcard ('*') as variable.

This switch is a consequence of having the same behavior than for block visibility pages.

Now for...

$valid_page = drupal_match_path(drupal_get_path_alias(), variable_get('nodejs_pages', ''));

By default then, $valid_page is always FALSE, because '' matches nothing at all. This is great because the default behavior assume that we'll refuse the acces to the listed pages in case of $valid_page returning TRUE.

I tested this in all possible options. Worked fine.
=> Please have another look at it and tell me !

Anonymous’s picture

Anonymous (not verified) commented

i'd really like to see us use 'whitelist' and 'blacklist' here.

it's too easy to read that code wrong otherwise.

socialnicheguru’s picture

socialnicheguru commented

This patch works for me

socialnicheguru’s picture

socialnicheguru commented
Status: Needs work » Reviewed & tested by the community
socialnicheguru’s picture

socialnicheguru commented
StatusFileSize
new add_disable_nodejs_pages-2120805-15.patch2.43 KB

This needs to be rerolled

glekli’s picture

glekli commented
Status: Reviewed & tested by the community » Fixed

Wonderful, thank you. This has been committed.

I made the following tweaks to the patch:
- The fieldset element does not need a default value
- Renamed variable for clarity
- The default should remain the same as before, and the new option should default to 'only the listed' pages, so that the behavior does not change when people update the module=

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.