Uploaded image for project: 'phpBB'
  1. phpBB
  2. PHPBB-15266

Content visibility events do not allow what they describe




      Events in the content_visibility class are ill-defined.

      In some/most situations, trying to add a condition through those events will result in total failure.  And in a lot of others, it is simply impossible due to grouping and priorities.

      This needs some serious fixing.

      Also, the use of content_visibility across the phpbb software is random.  In some places it is used, while in others it is not.  And the concept of "visible" is, in a number of situations, treated as synonym for "approved", which extensions may need/want to redefine.

      One example: trying to make the unapproved posts visible by the poster, the code in the listener would be something like:


           * SQL to get content visibility for forum
           * @param object $event The event object
           * @return void
           * @event core.content_visibility_get_visibility_sql_before
          public function get_visibility_sql_before($event)
              $event['where_sql'] = '(' . $event['table_alias'] . $event['mode'] . '_visibility = ' . ITEM_UNAPPROVED . ' AND ' .
                                  $event['table_alias'] . (($event['mode'] == 'post') ? 'poster_id = ' : 'topic_poster = ') .
                                  (int) $this->user->data['user_id'] . ') OR ';


       The event description says:


              * Allow changing the result of calling get_visibility_sql
              * @event core.phpbb_content_visibility_get_visibility_sql_before
              * @var    string        where_sql                        Extra visibility conditions. It must end with either an SQL "AND" or an "OR"
              * @var    string        mode                            Either "topic" or "post" depending on the query this is being used in
              * @var    array        forum_id                        The forum id in which the search is made.
              * @var    string        table_alias                        Table alias to prefix in SQL queries
              * @var    mixed        get_visibility_sql_overwrite    If a string, forces the function to return get_forums_visibility_sql_overwrite after executing the event
              *                                                     If false, get_visibility_sql continues normally
              *                                                     It must be either boolean or string
              * @since 3.1.4-RC1


      With this, the function correctly? returns (p.post_visibility = 0 AND p.poster_id = 59) OR p.post_visibility = 1, without surrounding parenthesis.

      But in case an OR is used (like the case above) the finally generated SQL when accessing a topic looks like:

      SELECT p.post_id
      FROM phpbb_posts p
      WHERE p.topic_id = 165 AND
                   (p.post_visibility = 0 AND p.poster_id = 59) OR p.post_visibility = 1
      ORDER BY p.post_time ASC, p.post_id ASC

      That given the operator priorities, will give back the list of ALL approved posts in any topic.

      Another example, in the  core.phpbb_content_visibility_get_global_visibility_before event.  Event description:


              * Allow changing the result of calling get_global_visibility_sql
              * @event core.phpbb_content_visibility_get_global_visibility_before
              * @var    array        where_sqls                            The action the user tried to execute
              * @var    string        mode                                Either "topic" or "post" depending on the query this is being used in
              * @var    array        exclude_forum_ids                    Array of forum ids the current user doesn't have access to
              * @var    string        table_alias                            Table alias to prefix in SQL queries
              * @var    array        approve_forums                        Array of forums where the user has m_approve permissions
              * @var    string        visibility_sql_overwrite    Forces the function to return an implosion of where_sqls (joined by "OR")
              * @since 3.1.3-RC1


      Apart that the documentation is totally wrong (visibility_sql_overwrite MUST be a string, and does not force an implosion or anything, while where_sqls is not an action, but an empty array, that MUST be filled in taking into account that will be converted to string by imploding it with "OR"), also the code is wrong: in case an extension adds a where_sqls element, and there are no moderated forum, the function returns:

              // There is only one element, so we just return that one
              return $where_sqls[0];

      Wrong, because it will be returning whatever the extension has put into the $where_sqls array, WITHOUT taking into consideration the rest of the function.

      There are more examples (in core.phpbb_content_visibility_get_forums_visibility_before it is impossible to add the condition above due to grouping and ordering of the code), but I don't want to write a Bible here.




            Marc Marc
            javiexin javiexin [X] (Inactive)
            0 Vote for this issue
            1 Start watching this issue