PHPBB3-10620 - Quote tag improvement

[AmigoJack]

Code: Select all

[quote="admin" post_id="583396" time="1433029874" user_id="2"]
[quote=admin post_id=583396 time=1433029874 user_id=2]

Numbers don't need quotes. But strings do - otherwise I'm the one signing up with the username a post_id=. Make it

Code: Select all

[quote="admin" post_id=583396 time=1433029874 user_id=2]

Also this was already an idea being discussed.

1. Post ID should be optional, aswell as username already is.
2. In the proposed format it will collide with usernames (think of a name like 3, 14) - far better formats would be these:

   Code: Select all

   [quote][/quote]             = No reference
   [quote="name"][/quote]      = Freely chosable name
   [quote="name",1337][/quote] = Freely chosable name and post ID
   [quote=404][/quote]         = Reference to user ID
   [quote=404,1337][/quote]    = Reference to user ID and post ID

3. I wonder how to distinguish between quoting a post in a PM and quoting a PM in a post.

[JoshyPHP]

Numbers don't need quotes. But strings do [...]

Technically speaking there are very few strings that need quotes in the new parser. Only a string with a ] or a string that contains something that looks like an attribute. I'm considering 3 different strategies for quoting attributes:

1. Everything in quotes
2. Only things that are not entirely made of digits
3. Only things that need to be in quotes, e.g. a username that contains a ]

Code: Select all

[quote="admin" post_id="583396" time="1433029874" user_id="2"]
[quote="admin" post_id=583396 time=1433029874 user_id=2]
[quote=admin post_id=583396 time=1433029874 user_id=2]

As far as the parser is concerned, they're all the same. It makes no difference.

[Pony99CA]

Again, if I'm missing something, please give a specific, detailed scenario where information may be leaked.

Create a new topic and preview [quote post_id=123][/quote] as its content. If you see a date and username in the quote then you know that post #123 exists, when it's been posted and who its author is, even if you don't have access to the forum it's in.

I would expect a "You don't have permission to view that forum" type of error if I don't have permission to view the forum that post is in.

I wasn't debating the meaning of words; I was just pointing out that they have different meanings to different people. You may think that this is simple, but it appears that Cin and I disagree. What's simple for you may not be simple for users.

I'm talking about code complexity, and adding attributes at quoting time is objectively simpler than doing it at posting time.

Now we're going in circles. I didn't dispute whether the coding was simpler; I was saying that only having post ID was simpler from the user's point of view. You argued that things were better for the user if the code was simpler (less chance of bugs), which is true to some degree, but that's why we have QA and Beta testing. Plus, with only a Post ID, you don't have to worry about the other attributes conflicting at all.

You have to keep the old school [quote="user-name"] option for backward compatibility and for people entering quotes manually, of course. (I'm presuming that having three different forms of quotes -- [quote], [quote="user-name"] and [quote=post_id] -- won't be a problem for the new parser.)

Steve

[Louis7777]

From the user's perspective I think it would be nice and simple if the quote BBCode could only take these forms:
[quote], [quote post_id=123] and [quote="user-name"]

Anything else could be problematic for the user.

Also, maybe the last two could be merged into one. By using double quotation for usernames and have it naked for post numbers (e.g. [quote=123]).

[AmigoJack]

As far as the parser is concerned, they're all the same. It makes no difference.

How does it distinguish between attribute="" and attribute= when I want to input "" as string?

Code: Select all

post_id= time= user_id=

Aren't those names longer than they need to be? It's obvious that IDs are used - but inconsistently time= doesn't hint its timestamp nature. Why not post= and user=?

Also, maybe the last two could be merged into one. By using double quotation for usernames and have it naked for post numbers (e.g. [quote=123]).

As I flagged multiple times: how to distinguish between PMs and posts? Both can use quotation and refer to an ID.

[JoshyPHP]

Attribute values are always strings. There's literally no difference between quoted and unquoted.

Aren't those names longer than they need to be? It's obvious that IDs are used - but inconsistently time= doesn't hint its timestamp nature. Why not post= and user=?

Originally it was post_time, before I realized it would be used for private messages too. So it got shortened to time. It could be named timestamp, I have no strong feelings either way. I named the other attributes after the column names of the values they contain, where applicable.

There's no message_id when quoting private messages. It doesn't seem like something people would use or need.

[AmigoJack]

There's literally no difference between quoted and unquoted.

My question was how then a string as "" (opposed to an empty string) can be detected, when it can mean both things: a quoted empty string, and an unquoted string with two quotation marks.

There's no message_id when quoting private messages. It doesn't seem like something people would use or need.

From my experience almost all people click "Quote" instead of "Reply", hence all the text is quoted. Which ends up in TOFU orgies. Anyway: PMs can be linked aswell - for every participant of a PM the same URI will be valid.

[JoshyPHP]

My question was how then a string as "" (opposed to an empty string) can be detected, when it can mean both things: a quoted empty string, and an unquoted string with two quotation marks.

The markup is unambiguous, x="" means that the attribute x has an empty string as value.

Double quotes are not allowed in usernames so it's entirely hypothetical, but if someone was named "" then the markup generated for the quote would use single-quotes, e.g. [quote='""']...[/quote]

PMs can be linked aswell - for every participant of a PM the same URI will be valid.

I'll have to leave that for a future PR.

[Nicofuma]

Merged and deployed as it on area51.

Test it intensively and feel free to create a ticket or open a discussion if there is something you don't like.

[AmigoJack]

1. Clicking the quote button creates the BBCode as intended:

   Code: Select all

   [quote=Nicofuma post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

   renders as:

   Merged

2. Robustness when skipping user ID in BBCode:

   Code: Select all

   [quote=Nicofuma post_id=281511 time=1436255398]Merged[/quote]

   as expected: username is not clickable anymore:

   Merged

3. Of course, this can now be used to spoof usernames which then link to other profiles:

   Code: Select all

   [quote=AmigoJack post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

   renders as:

   Merged

4. Trying to leave username empty:

   Code: Select all

   [quote= post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

   renders naively an empty A element (<a href="./memberlist.php?mode=viewprofile&u=111569"></a>), which can never be clicked:

   Merged

5. Username emptiness handling:

   1. Code: Select all

      [quote="" post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

      Merged

   2. Code: Select all

      [quote="""" post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

      [quote="""" post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

   3. Code: Select all

      [quote="''" post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

      Merged

   4. Code: Select all

      [quote='""' post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

      Merged

   5. Code: Select all

      [quote='' post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

      Merged

   6. Code: Select all

      [quote='0' post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

      Merged

6. Parameter commutativity:

   Code: Select all

   [quote=Nicofuma user_id=111569 time=1436255398 post_id=281511]Merged[/quote]

   works great:

   Merged

7. Parameter domination on duplicate attributes:

   Code: Select all

   [quote=Nicofuma post_id=281511 post_id=281166 time=1400000000 time=1436255398 user_id=111569 user_id=1 user_id=90321]Merged[/quote]

   Last one wins:

   Merged

8. Overflows, out of ranges:

   1. Code: Select all

      [quote=][ post_id=-1 time=-1 user_id=-1]Merged[/quote]

      [ post_id=-1 time=-1 user_id=-1]Merged

   2. Code: Select all

      [quote='][' post_id=-1 time=-1 user_id=-1]Merged[/quote]

      Merged

   3. Code: Select all

      [quote=[] post_id=100000000 time=100000000000 user_id=100000000]Merged[/quote]

      post_id=100000000 time=100000000000 user_id=100000000]Merged

   4. Code: Select all

      [quote="[]" post_id=100000000 time=100000000000 user_id=100000000]Merged[/quote]

      Merged

9. Interfering with make_clickable():

   Code: Select all

   [quote=" http://phpbb.com " post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

   Merged

10. Using different whitespaces:
    1. U+0009 = Tab:

       Code: Select all

       [quote=Nicofuma	post_id=281511	time=1436255398	user_id=111569]Merged[/quote]

       Merged

    2. U+000C = Form feed:

       Code: Select all

       [quote=Nicofumapost_id=281511time=1436255398user_id=111569]Merged[/quote]

       Merged

    3. U+000A = Line feed:

       Code: Select all

       [quote=Nicofuma
       post_id=281511
       time=1436255398
       user_id=111569]Merged[/quote]

       Merged

    4. U+000D = Carriage return:

       Code: Select all

       [quote=Nicofuma
       post_id=281511
       time=1436255398
       user_id=111569]Merged[/quote]

       Merged

    5. U+00A0 No-break space:

       Code: Select all

       [quote=Nicofuma post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

       Merged

    6. U+2007 = Figure space:

       Code: Select all

       [quote=Nicofuma post_id=281511 time=1436255398 user_id=111569]Merged[/quote]

       Merged