Commit graph

137 commits

Author SHA1 Message Date
Thiemo Kreuz 51ff3cc819 Several code cleanups after getting rid of cloning
* Use the Html class to safely create HTML code.
* $this->referenceStack can not be null any more.
* $this->inReferencesGroup is not needed during output, only when
  parsing tags.
* Replace ReferencesStack::getGroupRefs() as well as deleteGroup()
  with a combined popGroup() that does both things.
* Extract the code responsible for the "responsive" behavior to a
  separate function.
* Some TestingAccessWrapper are not needed.

Change-Id: Ie1cf2533d7417ae2f6647664ff1145e37b814a39
2019-12-16 15:47:23 +01:00
Adam Wight 9fd825e6a6 Simplify null comparison
In these cases, an expression is either true-ish or null, so we can use
the implicit boolean cast to test.

Change-Id: Ibe94829f9774bf2a1907635a8bd28369908b4d1e
2019-12-12 16:42:53 +01:00
Adam Wight ab07a3253c Remove Parser state from CiteErrorReporter
Finishes breaking the circular reference between Cite and Parser.

This patch also demonstrates how evil it is to allow the error reporter
to be called from anywhere, and have side-effects.  At least it's explicit
now.

Also fixes a bug where the inner error message would not be in the
interface language.

Bug: T240431
Change-Id: Ic3325cafb503e78295d72231ac6da5c121402def
2019-12-12 11:15:07 +01:00
Adam Wight 22b2f78db6 Remove Parser state from ReferencesFormatter
Bug: T240431
Change-Id: I3477a64b9a9dba0cfe890c4b598a51c2f971c76c
2019-12-12 11:12:17 +01:00
Adam Wight 2a5976f007 Remove Parser state from FootnoteMarkFormatter
Bug: T240431
Change-Id: Ie53444114c032e083293d3b5325252debb0640a7
2019-12-12 11:12:17 +01:00
Adam Wight 852a503262 Don't keep parser reference in Cite
This begins our journey of breaking the circular reference between
Cite and Parser.  In later patches the child objects will also take
Parser as a parameter.

Bug: T240431
Change-Id: Ic672bb4bae19ac5f1e1f5817de171d76b3bd8786
2019-12-12 11:12:17 +01:00
Adam Wight a227395e3a Lazy instantiation of Cite
Only create a Cite object if we need one.  Never clearState, just
destroy and recreate later.

This makes it less likely that we leak state between parsers, and
saves memory and processing on pages without references.

It's also preparation to decouple Cite logic from state.

Change-Id: I3db517591f4131c23151c76c223af7419cc00ae9
2019-12-12 11:12:17 +01:00
Thiemo Kreuz 3f2aeb7e31 Rename two Cite… classes and clean up test setups
* All classes are in a Cite\ namespace now. No need to repeat the word
"Cite" all over the place.

* The "key formatter" is more an ID or anchor formatter. The strings it
returns are all used in id="…" attributes, as well as in href="#…" links
to jump to these IDs.

* This patch also removes quite a bunch of callbacks from tests that
don't need to be callbacks.

* I'm also replacing all json_encode().

* To make the test code more readable, I shorten a bunch of variable
names to e.g. $msg. The fact they are mocks is still relevant, and still
visible because these variable names are only used in very short scopes.

Change-Id: I2bd7c731efd815bcdc5d33bccb0c8e280d55bd06
2019-12-12 08:48:02 +01:00
jenkins-bot 69a8754e31 Merge "Rename "index" parameter to "key"" 2019-12-11 16:25:30 +00:00
jenkins-bot 97e144755d Merge "Rename "key" variable to "lookup"" 2019-12-11 16:25:29 +00:00
jenkins-bot 0dafe64305 Merge "Rename CiteParserTagHooks::initialize to register" 2019-12-11 15:35:32 +00:00
jenkins-bot f642669522 Merge "Rename $type to $action in rollbackRef()" 2019-12-11 11:49:25 +00:00
jenkins-bot e39b1d6cbd Merge "Use messagelocalizer in CiteErrorReporter" 2019-12-11 11:42:28 +00:00
Adam Wight f93f1b4fe0 Use messagelocalizer in CiteErrorReporter
Fixes a bug introduced in Icf61c9a27fd, which would cause a parser
cache split any time the Cite extension was initialized.  The
`setLanguage` interface is regrettable, but I'm hoping it will only
be around temporarily.

Converts an integration test into a unit test and completes coverage.

Bug: T239988
Change-Id: I4b1f8909700845c9fa0cbc1a3de50ee7d42f69a5
2019-12-11 09:53:47 +01:00
jenkins-bot 76b4706938 Merge "Rename formatNumNoSeparators() to localizeDigits()" 2019-12-11 08:29:09 +00:00
jenkins-bot 37ea791a4c Merge "Use a guard clause in Cite::checkRefsNoReferences" 2019-12-11 08:18:18 +00:00
Thiemo Kreuz 66069d9dcf Use a guard clause in Cite::checkRefsNoReferences
This patch also cleans up a few pieces of PHPDoc documentation.

Change-Id: Ib207b11121769c543723db4668786f4916470368
2019-12-10 15:33:53 +00:00
Thiemo Kreuz 75016551e7 Rename formatNumNoSeparators() to localizeDigits()
Because that is what it does. Note our method is different from the one
in the Language class. We only accept strings.

Change-Id: I39107e837cc29f2d7c8867c1e602aa643f9e1a57
2019-12-10 16:21:12 +01:00
Thiemo Kreuz 83fc992159 Rename $type to $action in rollbackRef()
We call this an action when creating elements on the refCallStack.

Change-Id: I50e9df2f396060623e7e6b6deda086783709712b
2019-12-10 16:04:59 +01:00
Thiemo Kreuz df1a45b84c Fix incomplete cloning of the Parser::$extCite instance
This is what happens:
* The issue happens only on pages with two <ref> tags than share the
same name and group, but have conflicting text.
* This triggers a code path that renders an error message and calls
Message::plain() as well as Parser::addTrackingCategory(), which calls
Message::text().
* The Message class is asking for a new, fresh parser. This means the
parser is cloned and it's state cleared, while keeping stuff like
parser hooks.
* Cloning the parser triggers the ParserCloned hook.
* The hook handler clones the Cite instance stored in Parser::$extCite.
* PHP doesn't do deep cloning. Object properties are not cloned.
* Since I091a0b7 the internal state of the Cite class is extracted to
another class.
* This means the state is not cloned any more since I091a0b7.
* Now two Cite instances share the same state.
* At the end of the hook handler, the state is cleared, which also
clears the state of the original instance.

We will most probably solve this on master by getting rid of cloning
Cite. We propose this additional hotfix for the branch.

Bug: T240248
Change-Id: Ic5a438e04d003a637ae08aae936d9977cc90d5d3
2019-12-10 14:20:00 +00:00
Thiemo Kreuz 01bcfa773d Rename CiteParserTagHooks::initialize to register
It's called "register" in MediaWiki core as well.

Change-Id: Iad3dc3badbb7ad10a14276c3a144376acf70e5e5
2019-12-10 14:19:33 +00:00
Thiemo Kreuz 71c6dc7fe4 Better naming for ReferenceFormatter class and methods
This class renders a <references> tag and everything inside. The
previous name sounds like it is responsible for rendering the contents
of a <ref>…</ref> tag. I mean, the class contains a method that does
exactly this. But this method is private.

Change-Id: I1cd06c9a11e0a74104f2874a34efa3e0843a0f70
2019-12-10 08:40:09 +01:00
Thiemo Kreuz f92792f64a Fix bad localization of extended references numbers when reused
This adds a test for numbers like "1.2.0" that appear when an extended
reference (e.g. "1.2.") is reused multiple times.

The first separator is from the extended reference. We decided to never
localize it. However, the second seperator is from reusing a reference.
This was always localized. We believe this is a bug, but haven't fixed
it yet.

The test is documenting the status quo "1.2,0" with a comma. This kind
of makes sense, one could argue, because the "1.2" appears like this up
in the text, but the ",0" is a different indicator for a reuse, which
*never* occurs in the text.

Change-Id: Ie3d26bcadd8929b906bfbcac4806af2150d61f2a
2019-12-09 17:25:14 +01:00
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
jenkins-bot aa12b53e3e Merge "Comment about annoying thing" 2019-12-09 13:13:49 +00:00
Adam Wight 7e4ef22142 Rename "index" parameter to "key"
This is consistent, we check its value against $ref['key'].

Change-Id: Ie25edf4535893d3bb209920dfb3ebe089ee38cea
2019-12-09 13:26:22 +01:00
Adam Wight 4c045c897e Rename "key" variable to "lookup"
The concept "key" already exists in the structure handled by this
function, so to have a $key which means something else was distracting.

Change-Id: I91a76edbb42a1ab6514bc706b75ab89f78539fa5
2019-12-09 13:24:25 +01:00
jenkins-bot 740bd24178 Merge "Rename field to "key"" 2019-12-09 12:21:18 +00:00
jenkins-bot 4a0026d9bc Merge "Split validation function depending on inReferencesGroup" 2019-12-09 12:21:17 +00:00
Adam Wight 3e728799c2 Comment about annoying thing
Change-Id: I3c7f85bd822391d4e63314c0829ea1668d30a4ce
2019-12-09 12:56:58 +01:00
Adam Wight d6c0155e4c Rename field to "key"
This is consistent with the fact that it contains $ref['key'].

Change-Id: I134dba4a2405bb44b785e9cf191adc7bdd54c0d1
2019-12-09 12:52:26 +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
Adam Wight f51060eaf4 Fix footnote mark after extends numbering glitch
The visible numbering needed to be rolled back after an extends.

Bug: T237241
Change-Id: I95404515110df1fa7e3279ea499577df0ed45ddf
2019-12-09 12:06:59 +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
jenkins-bot 3b41cfa472 Merge "Fail early on nested extends="…", if possible" 2019-12-09 10:12:54 +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
jenkins-bot 238ed31d2e Merge "Add fail-safe default branch to switch-case" 2019-12-09 09:52:41 +00: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
Thiemo Kreuz 92312b4421 Add fail-safe default branch to switch-case
Reported as a possible code-smell at sonarcloud.io.

Change-Id: I3c5c1ff116dabe06c3d2e3cc59850ad3c66f8f83
2019-12-06 14:30:40 +01:00
Adam Wight 3d80501829 Narrow message localizer interface
We never access Language directly, so proxy its method instead of
returning the full object.

I believe I've found a bug, but not fixing here: the footnote body
numeric backlinks like "2.1" behave as if they were decimals rather
than two numbers stuck together with a dot.  So they are localized
to "2,1".

Bug: T239725
Change-Id: If386bf96d48cb95c0a287a02bedfe984941efe30
2019-12-06 12:17:09 +01:00
jenkins-bot 9622c4fb8f Merge "Comments to help understand the message localizer" 2019-12-05 14:49:58 +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
jenkins-bot 6255ab85d9 Merge "Drop unused variable" 2019-12-05 10:10:20 +00:00
jenkins-bot faf0b38fd9 Merge "Add visual whitespace to concat code" 2019-12-05 10:08:47 +00:00
Adam Wight b575835c63 Comments to help understand the message localizer
Change-Id: Ic7c9a12a78f358d11d997abf9a3a8e996f451c8f
2019-12-05 09:06:37 +01:00
Adam Wight 646dc5f974 Drop unused variable
Change-Id: I393a89ca909a632729c88ff73543bcf71061a8bf
2019-12-05 09:03:17 +01:00