A small gap I noticed when using this module on a client site with a layout manager like display suite was that there is no check on the nodes view mode when the code in hook_node_view runs. This manifested itself by nodes rendering in a non-full layout (eg: custom teaser, sidebar, whatever) either causing the parent page to become password protected when a referenced node with protection was rendered in a sidebar or the protected node would render in a limited view mode (like search result).

I think it's unfair to impose a patch that assumes you use a layout manager, but I did write a small patch that supports DS so that I can pick which view modes to enforce password protection on. It'd be great if someone could either extend or adapt as required, but I have no immediate expectations or requests for this to be merged in or not.

Thanks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

johan.gant’s picture

Grimreaper’s picture

Status: Active » Needs review

Hello,

Thanks for your contribution. I will test it this weekend.

I pass the status to needs review to trigger the automated tests.

I think you can filter on view modes like you have done without relying on DS.

Status: Needs review » Needs work

The last submitted patch, 1: protected-node-check-view-modes-2331517.patch, failed testing.

izus’s picture

hi,
i didn't think in depp to this but let's keep in mind that usually people protect a node because of data sensibility of some of it's fields. it may lead to bad surprised if some view mods display them.

Grimreaper’s picture

Status: Needs work » Needs review
FileSize
4.24 KB

Hello,

Here is a patch that doesn't require Display Suite for what you need to do.

If the admin does not select the view modes he/she wants to be protected, all node view modes are protected.

@johan.gant : The warnings from your patch come from the fact that you have inverted haystack and needle in your in_array in your check function.

@Izus : do you want that I implement tests for this new feature ?

Status: Needs review » Needs work

The last submitted patch, 5: protected_node_doesn_t-2331517-5.patch, failed testing.

Grimreaper’s picture

Status: Needs work » Needs review
FileSize
4.24 KB
izus’s picture

Status: Needs review » Needs work

Thanks both for the work done.
as suggested by @Grimreaper, i think that tests are a "good to have" here. so that we wil feel confortable with other new patches :)

johan.gant’s picture

@Grimreaper sorry about that, clearly wrote the original patch in haste and didn't spot the order of params. Thanks for the revised patches too, very useful.

@izus: your point about being careful with which viewmodes expose certain fields is valid. However, I think it's reasonable to limit this to a caveat for people who do use layout engines like ds or panels instead of not providing the flexibility in the first place.

Grimreaper’s picture

Status: Needs work » Needs review
FileSize
10.51 KB

Hello,

I have added two tests : one for allowed user, one for not allowed user. Both can see a node in an unprotected view mode.

For assertresponse method the $this->group is useless (not taken into account).

Thanks for the review.

  • izus committed ccc9c7b on 7.x-1.x authored by Grimreaper
    Issue #2331517 by Grimreaper, johan.gant: Added Protected node doesn't...
izus’s picture

Status: Needs review » Fixed

Hi,
Thanks both for comments and code.
I merged #10 but here are some extra notices, i'm not sure if they are all relevant.

  1. +++ b/tests/protected_node.view_mode.test
    @@ -0,0 +1,107 @@
    +   * Test that a promoted protected node can be seen on the front page
    

    We may want to be precise here : we are testing the /node page.

  2. +++ b/tests/protected_node.view_mode.test
    @@ -0,0 +1,107 @@
    +    $this->drupalGet('');
    

    i guess this is more precise
    $this->drupalGet('node');

Grimreaper’s picture

Hi,

Thanks for merging.

If you want to modify the code to be more precise, ok, I see no problem.

Status: Fixed » Closed (fixed)

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