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

improve quasi-documentation of notify_status values

    Details

    • Type: Improvement
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.7-PL1
    • Fix Version/s: 3.0.8-RC1
    • Component/s: Posting
    • Labels:
      None
    • Environment:
      all

      Description

      the sql in for instance includes/functions_posting.php at l1237, l1349, l1358 contains magic numbers for the notify_status column of forums_watch table, it would be nicer to implement at least define'd constants such as NO_NOTIFY, NOTIFY_YES, for legibility; there is a forum post that refers to the correct values, as well of course as the wiki; however given that [as well as the possibly counter-intuitive fact that 0=notify, 1=no notify] define'd constants are generally implemented in phpBB to avoid magic numbers it would be an improvement to replace the magic numbers; this may affect additional tables / notifications too

        Activity

        Hide
        markiemark1 markiemark1 added a comment -

        Hi

        as it seems a reasonably straightforward feature, I've written a patch; simplest would be it seems to simply add it to the git repo myself, so I've sent a message to the phpbb account as it looks as though I need permission to do that?

        Best regards

        Mark

        Show
        markiemark1 markiemark1 added a comment - Hi as it seems a reasonably straightforward feature, I've written a patch; simplest would be it seems to simply add it to the git repo myself, so I've sent a message to the phpbb account as it looks as though I need permission to do that? Best regards Mark
        Hide
        naderman Nils Adermann added a comment -

        You can push the patch to your own github repository (not the phpbb one) and then post a link here so someone will merge it in. We don't allow direct access to the repository so patches can be reviewed prior to being commited.

        Show
        naderman Nils Adermann added a comment - You can push the patch to your own github repository (not the phpbb one) and then post a link here so someone will merge it in. We don't allow direct access to the repository so patches can be reviewed prior to being commited.
        Hide
        markiemark1 markiemark1 added a comment - - edited

        Hi Nils,

        thanks for writing back so quickly; I was working from the instructions at http://wiki.phpbb.com/Working_with_Git "starting a new feature" so I suppose I'll have to learn some more gittery

        Now I see the purpose of forking the project, that had sounded kind of radical originally

        Show
        markiemark1 markiemark1 added a comment - - edited Hi Nils, thanks for writing back so quickly; I was working from the instructions at http://wiki.phpbb.com/Working_with_Git "starting a new feature" so I suppose I'll have to learn some more gittery Now I see the purpose of forking the project, that had sounded kind of radical originally
        Hide
        naderman Nils Adermann added a comment -

        Yeah I suppose that is still somewhat incomplete. What you want to do is fork the phpbb3 repository on github and push your changes to your own fork. origin is your repository then, not phpbb, and upstream is the phpbb repository, as mentioned in the guide.

        Show
        naderman Nils Adermann added a comment - Yeah I suppose that is still somewhat incomplete. What you want to do is fork the phpbb3 repository on github and push your changes to your own fork. origin is your repository then, not phpbb, and upstream is the phpbb repository, as mentioned in the guide.
        Hide
        markiemark1 markiemark1 added a comment -

        Hi,

        well git is quite straightforward after all

        as the new branch is 'all in one commit' the only noteworthy aspect is that in includes/functions_display.php at l1115 I improved the test to a non-strict [untyped] comparison [sql field <-> define'd constant] to NOTIFY_YES as there is already a test for null/empty string

        http://github.com/MarkieMark/phpbb3/tree/feature/notify_status

        Best regards

        Mark

        Show
        markiemark1 markiemark1 added a comment - Hi, well git is quite straightforward after all as the new branch is 'all in one commit' the only noteworthy aspect is that in includes/functions_display.php at l1115 I improved the test to a non-strict [untyped] comparison [sql field <-> define'd constant] to NOTIFY_YES as there is already a test for null/empty string http://github.com/MarkieMark/phpbb3/tree/feature/notify_status Best regards Mark
        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        Just changing status.

        Show
        Oleg Oleg [X] (Inactive) added a comment - Just changing status.
        Hide
        naderman Nils Adermann added a comment -

        Modified the commit message a tiny bit.

        Show
        naderman Nils Adermann added a comment - Modified the commit message a tiny bit.
        Hide
        bantu Andreas Fischer added a comment -

        One empty newline too much.

        Show
        bantu Andreas Fischer added a comment - One empty newline too much.

          People

          • Assignee:
            bantu Andreas Fischer
            Reporter:
            markiemark1 markiemark1
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development