Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-12886

Redundant lock and unlock expressions for QuickMod in viewtopic.php

    Details

      Description

      And it seems to be unintentional:
      *lock*

      'lock'					=> array('LOCK_TOPIC', ($topic_data['topic_status'] == ITEM_UNLOCKED) && ($auth->acl_get('m_lock', $forum_id) || ($auth->acl_get('f_user_lock', $forum_id) && $user->data['is_registered'] && $user->data['user_id'] == $topic_data['topic_poster'] && $topic_data['topic_status'] == ITEM_UNLOCKED))),

      Line breaks:

      'lock'					=> array('LOCK_TOPIC', 
      	($topic_data['topic_status'] == ITEM_UNLOCKED) &&
      	(
      		$auth->acl_get('m_lock', $forum_id) ||
      		(
      			$auth->acl_get('f_user_lock', $forum_id) &&
      			$user->data['is_registered'] &&
      			$user->data['user_id'] == $topic_data['topic_poster'] &&
      			$topic_data['topic_status'] == ITEM_UNLOCKED
      		)
      	)
      ),

      *unlock*

      'unlock'				=> array('UNLOCK_TOPIC', ($topic_data['topic_status'] != ITEM_UNLOCKED) && ($auth->acl_get('m_lock', $forum_id) || ($auth->acl_get('f_user_lock', $forum_id) && $user->data['is_registered'] && $user->data['user_id'] == $topic_data['topic_poster'] && $topic_data['topic_status'] == ITEM_UNLOCKED))),

      Line breaks:

      'unlock'				=> array('UNLOCK_TOPIC',
      	($topic_data['topic_status'] != ITEM_UNLOCKED) &&
      	(
      		$auth->acl_get('m_lock', $forum_id) ||
      		(
      			$auth->acl_get('f_user_lock', $forum_id) &&
      			$user->data['is_registered'] &&
      			$user->data['user_id'] == $topic_data['topic_poster'] &&
      			$topic_data['topic_status'] == ITEM_UNLOCKED
      		)
      	)
      ),

      The second topic status in the lock check is redundant.
      The second one in the unlock check is never true, due to the first topic status comparison. I think it should be != ITEM_UNLOCKED there aswell and then its redundant and can be removed aswell.

      If we do not want f_user_lock to be able to unlock topics, it should be

      'unlock'				=> array('UNLOCK_TOPIC', $topic_data['topic_status'] != ITEM_UNLOCKED) && $auth->acl_get('m_lock', $forum_id)),

      instead.
      But the current statement is just non-sense

        Activity

        Hide
        nickvergessen Joas Schilling added a comment -

        Seems like f_user_lock is designed to not unlock topics:
        https://github.com/phpbb/phpbb/blob/develop-olympus/phpBB/viewtopic.php#L562
        3.0 logic:

        $topic_mod .= ($auth->acl_get('m_lock', $forum_id) || ($auth->acl_get('f_user_lock', $forum_id) && $user->data['is_registered'] && $user->data['user_id'] == $topic_data['topic_poster'] && $topic_data['topic_status'] == ITEM_UNLOCKED)) ? (($topic_data['topic_status'] == ITEM_UNLOCKED) ? '<option value="lock">' . $user->lang['LOCK_TOPIC'] . '</option>' : '<option value="unlock">' . $user->lang['UNLOCK_TOPIC'] . '</option>') : '';
        

        line breaks

        $topic_mod .= 
        (
        	$auth->acl_get('m_lock', $forum_id) ||
        	(
        		$auth->acl_get('f_user_lock', $forum_id) &&
        		$user->data['is_registered'] &&
        		$user->data['user_id'] == $topic_data['topic_poster'] &&
        		$topic_data['topic_status'] == ITEM_UNLOCKED
        	)
        ) ? (($topic_data['topic_status'] == ITEM_UNLOCKED) ? '<option value="lock">' . $user->lang['LOCK_TOPIC'] . '</option>' : '<option value="unlock">' . $user->lang['UNLOCK_TOPIC'] . '</option>') : '';
        

        So we should change it to

        => array('UNLOCK_TOPIC', $topic_data['topic_status'] != ITEM_UNLOCKED) && $auth->acl_get('m_lock', $forum_id)),

        Show
        nickvergessen Joas Schilling added a comment - Seems like f_user_lock is designed to not unlock topics: https://github.com/phpbb/phpbb/blob/develop-olympus/phpBB/viewtopic.php#L562 3.0 logic: $topic_mod .= ($auth->acl_get('m_lock', $forum_id) || ($auth->acl_get('f_user_lock', $forum_id) && $user->data['is_registered'] && $user->data['user_id'] == $topic_data['topic_poster'] && $topic_data['topic_status'] == ITEM_UNLOCKED)) ? (($topic_data['topic_status'] == ITEM_UNLOCKED) ? '<option value="lock">' . $user->lang['LOCK_TOPIC'] . '</option>' : '<option value="unlock">' . $user->lang['UNLOCK_TOPIC'] . '</option>') : ''; line breaks $topic_mod .= ( $auth->acl_get('m_lock', $forum_id) || ( $auth->acl_get('f_user_lock', $forum_id) && $user->data['is_registered'] && $user->data['user_id'] == $topic_data['topic_poster'] && $topic_data['topic_status'] == ITEM_UNLOCKED ) ) ? (($topic_data['topic_status'] == ITEM_UNLOCKED) ? '<option value="lock">' . $user->lang['LOCK_TOPIC'] . '</option>' : '<option value="unlock">' . $user->lang['UNLOCK_TOPIC'] . '</option>') : ''; So we should change it to => array('UNLOCK_TOPIC', $topic_data['topic_status'] != ITEM_UNLOCKED) && $auth->acl_get('m_lock', $forum_id)),
        Hide
        Elsensee Oliver Schramm added a comment -

        But I think it should though. f_user_lock can be given to users so they can lock their own topics as we now. But for me with the right to lock topics also comes the right to unlock topics...

        Show
        Elsensee Oliver Schramm added a comment - But I think it should though. f_user_lock can be given to users so they can lock their own topics as we now. But for me with the right to lock topics also comes the right to unlock topics...
        Hide
        Volksdevil Volksdevil added a comment -

        ^^ Agreed, although I'd be happy to see both permission examples:

        1/ Users can both 'LOCK' and 'UNLOCK' topics
        OR
        2/ Users can 'LOCK' and/or 'UNLOCK' topics.

        Show
        Volksdevil Volksdevil added a comment - ^^ Agreed, although I'd be happy to see both permission examples: 1/ Users can both 'LOCK' and 'UNLOCK' topics OR 2/ Users can 'LOCK' and/or 'UNLOCK' topics.
        Hide
        Volksdevil Volksdevil added a comment -

        Confused about the pull request/file change...I just applied it and a member still can't unlock their own topics which they have locked?

        Show
        Volksdevil Volksdevil added a comment - Confused about the pull request/file change...I just applied it and a member still can't unlock their own topics which they have locked?
        Hide
        bantu Andreas Fischer added a comment - - edited

        This ticket and the PR are about removing unused code. This does not involve a behaviour change in any way.

        Show
        bantu Andreas Fischer added a comment - - edited This ticket and the PR are about removing unused code. This does not involve a behaviour change in any way.

          People

          • Assignee:
            Marc Marc
            Reporter:
            nickvergessen Joas Schilling
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development