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
Comment | File | Size | Author |
---|---|---|---|
#10 | protected_node_doesn_t-2331517-10.patch | 10.51 KB | Grimreaper |
#1 | protected-node-check-view-modes-2331517.patch | 2.51 KB | johan.gant |
Comments
Comment #1
johan.gant CreditAttribution: johan.gant commentedComment #2
GrimreaperHello,
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.
Comment #4
izus CreditAttribution: izus commentedhi,
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.
Comment #5
GrimreaperHello,
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 ?
Comment #7
GrimreaperComment #8
izus CreditAttribution: izus commentedThanks 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 :)
Comment #9
johan.gant CreditAttribution: johan.gant commented@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.
Comment #10
GrimreaperHello,
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.
Comment #12
izus CreditAttribution: izus commentedHi,
Thanks both for comments and code.
I merged #10 but here are some extra notices, i'm not sure if they are all relevant.
We may want to be precise here : we are testing the /node page.
i guess this is more precise
$this->drupalGet('node');
Comment #13
GrimreaperHi,
Thanks for merging.
If you want to modify the code to be more precise, ok, I see no problem.