Term rules oddity

  • Resolved peter8nss

    (@peter8nss)


    3 months, 3 weeks ago

    I don’t think “is_post_type” is working correctly for term restrictions.

    Consider the following pair of restrictions:

    • 1 Internal Pages
      • General: Logged in users
      • Protection: Redirect
      • Content: “Content is Page” AND “Content is Not Home Page”
    • 2 Term
      • General: Logged in users of a role, say “Author”
      • Protection: Redirect
      • Content: “Content is selected Tag = xxx”

    I have a page that triggers a query involving a post_tags. When I’m logged in as a role other than “Author” I would expect the Tag xxx to be removed from the query – because of restriction 2. However, restriction 2 never gets tested. The query matches against restriction 1!

    When “content_is_post_type” for “page” is called to test first part of restriction 1 for context “terms” it goes through the default branch which calls is_post_type which succeeds.

    I think the switch in “content_is_post_type” needs the following additional case statement:

    case 'terms':
         return false;
Viewing 3 replies - 1 through 3 (of 3 total)
  • Plugin Author Daniel Iser

    (@danieliser)

    In this case your running into a performance optimization. By default only the first matched restriction is used (sorted by priority of restriction sets). This prevents needless extra rule checks etc.

    To process all of them, in cases where a user might match more than one at a time, you can enable processing of all restrictions:

    https://contentcontrolplugin.com/docs/troubleshooting/basic-troubleshooting-guide/#h-4-my-top-restrictions-stop-or-short-circuit-ones-lower-in-the-restriction-list

    The filter there will do what your after, processing the secondary restrictions as well.

    —

    Keep in mind 98%+ of our users only have one restriction set up for any given user role/type match, so that is what we optimize for, but we do offer the flexibility to do it the other way as well for those that need it.

    • This reply was modified 3 months, 3 weeks ago by Daniel Iser.
    Thread Starter peter8nss

    (@peter8nss)

    I don’t think it is to do with optimisation. Yes the page should display (and does becuase it matches the first restriction), but when the query on that page is being checked, it should NOT match the first restriction, because it is query term that is being checked and hence should not match “is a Page”. And should go on to test the second restriction (which it would match) and whether the query was adjusted would depend on what the user’s role was.

    Once you have the fixes in place for Query Monitor it is a bit easier to see.

    Plugin Author Daniel Iser

    (@danieliser)

    @peter8nss – I think I see the issue.

    We have “default” switch cases currently, I think removing them, and returning false by default on fall through (no switch case match) should resolve it efficiently.

    Adding a case 'terms': return false, effectively does the same thing, explicitly, but I don’t think explicitly is necessary if we remove the default: and fail gracefully for all non matches.

    Our goal is to minimize case matches, rather than match every option. Mainly for maintainability, supposing we add new contexts in the future for example.

Viewing 3 replies - 1 through 3 (of 3 total)
  • You must be logged in to reply to this topic.