Commit graph

65 commits

Author SHA1 Message Date
Thiemo Kreuz 7c1849d7b0 Report both nested <ref> and <references> as an error
Before, this regular expression was looking for incomplete wikitext
like this:

<ref>unclosed
<ref>closed</ref>

With this change, wikitext like this will trigger the same error:

<ref>unclosed
<references />
incomplete</ref>

This should be much, much more rare. But I feel it's reasonable to mark
this as an error, instead of just rendering the broken inner tag in
plain text.

This patch also replaces `.*?>` with `[^>]*+>`. Both do the exact same.
Instead of doing an "ungreedy search for the first possible closing
bracket", which might cause backtracking, the new syntax consumes all
non-brackets before expecting one. This is guaranteed to never backtrack
(guaranteed by the extra +), and potentially faster because of this.

Change-Id: Ic76a52cd111b28e4522f095ce3984e3583f602c1
2019-12-09 14:26:28 +01:00
Adam Wight 8097c4c148 Split validation function depending on inReferencesGroup
Some validation is exclusively used in a specific context, some is shared.

Change-Id: I390db1c9d4854871e25a2e74411476e4e1c0b66f
2019-12-09 12:27:52 +01:00
jenkins-bot 0f0356ffc1 Merge "Refine and fix "unclosed <ref> detected" regular expression" 2019-12-09 10:29:13 +00:00
jenkins-bot a8e882e39f Merge "Show "Preview" headline in user instead of content language" 2019-12-09 10:13:17 +00:00
Thiemo Kreuz c5fe49ff11 Fail early on nested extends="…", if possible
This partly reverts Ied2e3f5. I haven't properly tested this before.
Rendering a bad extends (that extends a <ref> that's already extended)
not indented messes the order up and rips other extended <ref>s out of
context.

For now it might be better to stick to the previous, "magic" behavior:
Such an extends behaves like it is extending the *parent*, and is
ordered and indented as such. This is still not correct, but I feel
this is much better than rendering such a bad extends on the top level.

This patch also makes the code fail much earlier for a nested extends,
if this decision can be made already. In this case the error message is
rendered in the middle of the text (as other errors also are), not in
the <references> section.

Change-Id: I33c6a763cd6c11df09d10dfab73f955ed15e9d36
2019-12-09 10:54:52 +01:00
Thiemo Kreuz 8fdce945bd Show "Preview" headline in user instead of content language
This partly reverts Id7a4036e64920acdeccb4dfcf6bef31d0e5657ab.

The message "cite_section_preview_references" says "Preview of references".
This line is not meant to be part of the content, but an interface message.
It should use the users (interface) language, not the content language.

Change-Id: I1b1b5106266606eb0dfaa31f4abd3cee9ba92e8c
2019-12-09 10:53:07 +01:00
Thiemo Kreuz a7ee7c9586 Refine and fix "unclosed <ref> detected" regular expression
This simplifies as well as fixes a series of issues with this regular
expression:

* Before, the wikitext `<REF><REF>` would not trigger the error, but
`<ref><ref>` would. Parser tags are case-insensitive, but the error
check was not.

* Before, the wikitext `<ref><ref name="<">` would not trigger the error.
That's a valid name. The error check should not stop just because it
found a `<`.

* Both the old and the new code do *not* fail with the wikitext
`<ref><ref</ref>` where the inner `<ref` does not have a closing `>`. I
was thinking about changing this, but figured it might be used as a
feature.

* The old code was not able to properly understand HTML comments,
<nowiki> tags and such that contain a line break. That caused
inconsistent and confusing error reporting in some cases, but not in
others. This change *reduces* the amount of errors this code produces.

* The old code was looking for "SGML tags" with names that could be
anything, not just alphanumeric characters. This allowed for strange
edge-cases like `<ref><>><ref></>></ref>` that have not been reported,
but should be. This change *increases* the amount of errors. However,
relevant edge-cases should be extremely rare.

Note the ++ avoids backtracking, speeding up the regex.

Change-Id: I0c61a245f4f743871b4cad886ce239650af2b37c
2019-12-08 04:37:13 +00:00
Adam Wight 01c76f46a6 Use message localizer in CiteKeyFormatter
Makes more tests easier.

Change-Id: I222ba61bfcf0be3e29cb04e39f44f0be7a9e0778
2019-12-05 14:57:32 +01:00
Adam Wight 1ce4079ce2 Use message localizer in FootnoteMarkFormatter
Completes test coverage.

Change-Id: Ib2ec24cf4a9de52769744d1888cb13d2bf08ae3b
2019-12-05 14:56:53 +01:00
Adam Wight 430086cb6b Use the message localizer in Cite
Allows us to convert another integration test into a unit test.

Change-Id: Id7a4036e64920acdeccb4dfcf6bef31d0e5657ab
2019-12-05 13:23:31 +01:00
Adam Wight 646dc5f974 Drop unused variable
Change-Id: I393a89ca909a632729c88ff73543bcf71061a8bf
2019-12-05 09:03:17 +01:00
Adam Wight 5705228d17 Complete validateRef coverage
Change-Id: Id61fba34a8815a0c512ecf4bc57da3be4e15c8bb
2019-12-04 18:00:13 +01:00
Thiemo Kreuz a7c4e14f42 Remove obsolete ParserBeforeTidy hook handler
I was able to track this code down to I093d85d from 2012, which was done
right after the ParserAfterParse hook was introduced. I believe the
redundant code path was left to keep the Cite extension compatible with
old MediaWiki versions that did not had this hook yet.

I also noticed this code path is most probably entirely redundant with
the current version of MediaWiki. The *only* thing this code does is
blocking the ParserBeforeTidy hook from doing the same thing a second
time if the ParserAfterParse hook was called before. But it does *not*
block any other compination, e.g. if the two hooks are called the other
way around, or the same hook twice.

In core, it looks like it is impossible for the ParserBeforeTidy hook
being fired without the ParserAfterParse hook being fired before. If this
is true, this is in fact dead code.

Change-Id: Iacf8b600c7abdeaf89c22c2fc31e646f57245e47
2019-12-04 16:56:43 +01:00
Adam Wight c09d90aff3 Use message localizer in FootnoteBodyFormatter
Makes the class more easily testable.

Patch also changes an integration test into a unit test.

Change-Id: I545730404aceed7e3857d96f4fd3c1b0a900c0c2
2019-12-04 13:41:44 +01:00
Adam Wight d04cc36fa4 Replace reference parameter with return value
This makes it obvious that our function isn't sensitive to the input value.

Also rearranges a string concatenation to make the element wrapping clearer.
I probably should have switched to the HTML class here, but I'm not sure what
the advantages would be.

Change-Id: Ife3424ce68588f73f168b10e63e6cd81c4a60084
2019-12-04 11:28:38 +01:00
Adam Wight e9958d569b Formatter takes responsibility for rendering footnote mark
Pass the full ref structure from ReferenceStack to FootnoteMarkFormatter,
to give it control over the final rendering.  This is aligned with how
the FootnoteBodyFormatter directly scans over groupRefs.

Change-Id: I3294fd9366f01daa4250a5d481f4adbae84c72b1
2019-12-02 10:17:24 +01:00
Thiemo Kreuz 504db2c46a Add strict PHP 7 type hints to most code
This patch does two things:
* Add strict PHP 7 type hints to most code.
* Narrow the interface of the checkRefsNoReferences() method to not
  require a ParserOptions object any more.

Change-Id: I91c6a2d9b76915d7677a3f735ee8e054c898fcc5
2019-12-02 08:51:42 +01:00
Adam Wight ffedf86a19 Clean up text and name conditionals
The "no key" error should have been unreachable, but I'm afraid that
null `$text` and empty string `$text` were reporting slightly different errors.

Unfortunately, we still have to care about `$text = '0'` because PHP, so
the expressions don't reduce to `if ( !$text ...`

Change-Id: Id1028611ec3bc462dca413f31f7f59637bd7cc7b
2019-11-29 19:26:12 +00:00
Adam Wight a40b1b10be Extract footnote body rendering
Change-Id: I9537849cbd700d5dc7ec1a53d852d69b0fe0dc35
2019-11-29 16:22:35 +01:00
Adam Wight c236524138 Extract footnote mark rendering
Change-Id: I79de89e46da36dc1f0ee2b2fdb9a139e6434fde2
2019-11-29 16:22:35 +01:00
jenkins-bot 4b58a25459 Merge "Extract key formatting" 2019-11-29 15:19:52 +00:00
jenkins-bot 50ccd05def Merge "Render nested references" 2019-11-29 15:19:50 +00:00
Thiemo Kreuz f8affe8eee Remove redundant variable names from @var docs
Change-Id: I5ba2f1041d3d6770c58f496482d2799bb24786be
2019-11-29 14:37:58 +01:00
Adam Wight a4c056f59b Extract key formatting
Change-Id: I155ec6f3e21075587dbcfdfdc346f28f958e3c15
2019-11-29 13:41:12 +01:00
Thiemo Kreuz 13598ba11e Render nested references
Forked from Icd933fc983.

Bugs and unimplemented features are documented as TODOs in the parser test
fixtures.

Bug: T237241
Change-Id: I9427e025ea0bcf2fa24fd539a775429cc64767cc
2019-11-29 13:40:34 +01:00
jenkins-bot 3beb5c3634 Merge "Remove ApiQueryReferences support" 2019-11-29 11:30:38 +00:00
Adam Wight a176e22097 Remove ApiQueryReferences support
This API was never used in Wikimedia production, and would have caused
performance problems.  Removing the dead code will simplify our refactoring.

Bug: T238195
Change-Id: I7088f257ec034c0d089e0abdaa5a739910598300
2019-11-28 11:08:46 +01:00
Adam Wight ab78df8d5c Wire extends into ReferenceStack
Takes no action, just shuttle the value between functions.

(Split from I9427e025ea0)

Change-Id: I271043e9161835f3278098787bf58b50ed93c892
2019-11-28 02:10:11 +01:00
jenkins-bot dbf4c56896 Merge "[Refactor] Pass validation error with StatusValue" 2019-11-27 21:23:30 +00:00
jenkins-bot e4a961a6c1 Merge "Rename $valid to $status for clarity" 2019-11-27 21:23:29 +00:00
Adam Wight 22a0350d84 [Refactor] Pass validation error with StatusValue
This has clearer semantics than checking for a `false` attribute.

Change-Id: I68f777eda40f8f157deafacaed02d4bd10cbf25c
2019-11-27 18:05:19 +01:00
Thiemo Kreuz 99ee9e443b Rename $valid to $status for clarity
This also splits some code a little bit to make the next patch smaller.

Change-Id: Ibc02fa3d683043de86d21a7aa3feef373502552a
2019-11-27 17:51:22 +01:00
Adam Wight b30340ba2b Clean up pushRef
This is motivated by I9427e025e, which demonstrates that the existing
logic was hard to integrate into.  There's a lot of redundant expressions
which make the function difficult to read, and code paths which have
less effect than they appear to.

Change-Id: Ida9612d1457f2593647b8fc02930d2e9ae824814
2019-11-27 16:40:51 +01:00
Thiemo Kreuz 0013943a4a Rewrite argument parsing and use for both <ref> & <references>
We realized the trim() are not needed. This does not leave much behind
in the existing refArg() method, except that it checks for unknown keys.

I tried a few strategies and ended using the pretty new possibility to
have keys in list(), as well as use [] instead of list(). Both is
supported since PHP 7.1.

Change-Id: I569bfa14e68b64402519bd39022c197553881dde
2019-11-27 14:01:52 +01:00
Thiemo Kreuz 99d23ac841 Remove redundant attribute trimming
We noticed the group="…" attribute was the only one that was not
trimmed. Does this mean it was possible to have two groups "a" and
" a"? It turns out: no. This was never possible because the parser
already trims all attributes before calling this code.

I tried to come up with the worst possible test case, but it succeeds,
even with very old versions of this codebase.

I suggest to remove the extra trimming from this codebase and rely on
what the parser provides.

Note the content is special and *not* trimmed by default.

Change-Id: Idff015447d7156ba7b5c03a5c423f199a71349f2
2019-11-27 12:12:51 +00:00
Adam Wight 7cdcc2b075 Alphabetize returned array of attributes
That was annoying me.  Since we're passing a bare list, alphabetical
order helps make the code and tests readable.

Change-Id: I6384094e429e0e2a6fa810fdc28ae0643a0ccf7c
2019-11-26 15:28:02 +01:00
Adam Wight 6a9e51fc30 Tag every usage of "extends" even when invalid
We weren't recording usages which failed certain `refArg` validations.

Bug: T237531
Change-Id: Ibcb875c5d0ed6c2279e0e34ab415ac63d7ebe584
2019-11-26 15:28:02 +01:00
Adam Wight 2229f22899 [Refactor] Nest conditionals with common term
Previewing is an important exception, so I wanted to consolidate and
emphasize its edge cases.

Change-Id: Iae343ed8c225407e8184ff09e426d531c9f6ab00
2019-11-26 15:28:02 +01:00
Adam Wight 301b1fbcaa Move a follow edge case to validation
Change-Id: I06cf5291c258322e16449d61879bf7a18129b174
2019-11-26 15:28:02 +01:00
Adam Wight 44a2599ab4 [Refactor] Handle extends attribute in validation
Moves logic so that `refArg` handles parsing and `validateRef` more
of the validation.

Change-Id: I2c0a789d5f2c20b1968c4809b5780d9fe738fd9a
2019-11-26 15:28:02 +01:00
Adam Wight d1e0e6309b Add some comments
Change-Id: I012ad6cc52dc65f1b329febdc8e6441ac03c6463
2019-11-26 11:07:48 +01:00
Adam Wight 9fa22b15be Signature: these are all nullable
Change-Id: I79432de9007f9016604cb871b68577f1cf596be5
2019-11-26 11:07:39 +01:00
Adam Wight d65bb667b7 Drop unused param
The "dir" param is not validated, so don't pass through.

Change-Id: I78eaa3bf7067c6283a2d8e452b68f93ffab43875
2019-11-26 11:07:39 +01:00
Adam Wight 33014bb8f5 Comments in TODOs
Stuff we have to fix in future work.  Follows up on I2d9904b7631d0d6

Change-Id: I7bca73d3e9d3d2a224604efbe81d48948d2a3d76
2019-11-26 11:07:12 +01:00
Adam Wight 22b0bdf526 Test for numeric extends attribute
This is invalid, because it would allow access to internal, autoindexed,
anonymous refs.  These would break when refs are reordered.

Bug: T151305
Change-Id: Ib4bb8270d810b64e4c160f377ce52ce2fc70bab4
2019-11-26 11:07:11 +01:00
Adam Wight feaa724efa Test for numeric attributes earlier
This introduces a slight behavior change, but for the better:
* When pointing to the name "0", the non-numeric error will be displayed,
  which is correct whereas "no key" is not.

Change-Id: I33467b27cd447812fe67204831909c4d9869db08
2019-11-26 11:06:15 +01:00
Adam Wight 3fbeed0304 [Refactor] Consolidate validation
Validation logic can be split from arg parsing, default values and
other side-effects.

No behavior was changed.

Change-Id: I2d9904b7631d0d6be13e0aaed0106f186d388c4f
2019-11-26 09:25:06 +01:00
Adam Wight 9e2468882d Cache parser previewing state
We need to access this in several places, store as instance state for
convenience.

Change-Id: I4ea8f279a34cd8f819d9c07c75e3e8e160786f9b
2019-11-25 14:06:55 +01:00
Adam Wight 8453e3ecd7 Extract stack and state to a new class
Most of this state is used to manage interactions with other state,
and encapsulation allows us to hide data structures and access behind
self-explanatory function names.

The interface is still much wider than I'd like, but it can be improved in
future work.

There is one small behavior change in here: in the `follows` edge case
demonstrated by I3bdf26fd14, we prepend if the splice point cannot be
used because it has a non-numeric key.  I believe this was the original
intention of the logic, and is how the numeric case behaves.  I've verified
that when array_splice throws a warning about non-numeric key, it fails to
add anything to the original array, so the broken follows ref disappeared.

Bug: T237241
Change-Id: I091a0b71ee9aa78e841c2e328018e886a7217715
2019-11-25 14:06:32 +01:00
Adam Wight 10e4a4353d Finish renaming "key" to "name"
Change-Id: Iffd49268cfefefdce9c970f60b5d231e79cbc267
2019-11-25 13:25:40 +01:00