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

Double clicking "mark topics read" produces an error the second time

    Details

      Description

      http://localqi/boards/x33/viewforum.php?f=11

      Double click "mark topics read" link. The first request succeeds, the second one produces the nondescript failure message.

      The failure message is hidden automatically on timeout (http://tracker.phpbb.com/browse/PHPBB3-11281).

        Issue Links

          Activity

          Hide
          Marc Marc added a comment -

          This is due to the fact that we select rows from the database to update in the forums track table like this:

          					AND mark_time < $post_time

          If we try marking the forum at the same time, this causes us to do an insert instead of an update. Modifying it to this fixes our issue:

          					AND mark_time <= $post_time

          This needs to be done twice. Once here: https://github.com/phpbb/phpbb3/blob/develop/phpBB/includes/functions.php#L1407
          And another time here: https://github.com/phpbb/phpbb3/blob/develop/phpBB/includes/functions.php#L1423

          Show
          Marc Marc added a comment - This is due to the fact that we select rows from the database to update in the forums track table like this: AND mark_time < $post_time If we try marking the forum at the same time, this causes us to do an insert instead of an update. Modifying it to this fixes our issue: AND mark_time <= $post_time This needs to be done twice. Once here: https://github.com/phpbb/phpbb3/blob/develop/phpBB/includes/functions.php#L1407 And another time here: https://github.com/phpbb/phpbb3/blob/develop/phpBB/includes/functions.php#L1423
          Hide
          EXreaction EXreaction [X] (Inactive) added a comment -

          Actually, this should be fixed by just removing this line:
          https://github.com/phpbb/phpbb3/blob/develop/phpBB/includes/functions.php#L1407

          That way it would select all the rows, update only the ones needed, and not insert any where sql errors would occur.

          Also, the same issue could occur on topics. In this section:
          https://github.com/phpbb/phpbb3/blob/develop/phpBB/includes/functions.php#L1492

          A select query should be run before the insert to make sure a row does not exist already.

          Show
          EXreaction EXreaction [X] (Inactive) added a comment - Actually, this should be fixed by just removing this line: https://github.com/phpbb/phpbb3/blob/develop/phpBB/includes/functions.php#L1407 That way it would select all the rows, update only the ones needed, and not insert any where sql errors would occur. Also, the same issue could occur on topics. In this section: https://github.com/phpbb/phpbb3/blob/develop/phpBB/includes/functions.php#L1492 A select query should be run before the insert to make sure a row does not exist already.
          Hide
          bantu Andreas Fischer added a comment -

          Surely should wrap all four queries into a transaction.

          Show
          bantu Andreas Fischer added a comment - Surely should wrap all four queries into a transaction.
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          What is the actual error that is being produced?

          Show
          Oleg Oleg [X] (Inactive) added a comment - What is the actual error that is being produced?
          Hide
          EXreaction EXreaction [X] (Inactive) added a comment -

          Andreas, I thought we didn't want transactions for this sort of stuff (we removed them for bookmark/subscription handling).

          Show
          EXreaction EXreaction [X] (Inactive) added a comment - Andreas, I thought we didn't want transactions for this sort of stuff (we removed them for bookmark/subscription handling).
          Hide
          Marc Marc added a comment -

          This doesn't seem to be possible anymore. I'm unable to reproduce this on area51.

          Show
          Marc Marc added a comment - This doesn't seem to be possible anymore. I'm unable to reproduce this on area51.
          Hide
          prototech prototech added a comment -

          I can reproduce it locally. See attached screenshot.

          Show
          prototech prototech added a comment - I can reproduce it locally. See attached screenshot.

            People

            • Assignee:
              EXreaction EXreaction [X] (Inactive)
              Reporter:
              Oleg Oleg [X] (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development