This patch is mostly moving code around without changing the behavior.
Exceptions:
* The ErrorReporter creates a <span> container. This was previously
parsed. The only benefit might be error checking and escaping. Rather
pointless. The code just created this HTML. With this patch, it is not
parsed any more. The unit test reflects this change. The output in
production will not change, as the parser tests show.
* Parsing of the message key (to detect it's type and id) is simplified
a lot, using explode. With this the code can, in theory, support more
types.
Bug: T239572
Change-Id: If2fe5f55db46dfc7e0ce445348608bef00bec64e
Perform the validation in validateRef, and display a new error message for
broken "follow" refs. This changes existing behavior, where broken folow
ref content is arbitrarily displayed at the top of the references list and
no error is rendered.
Thanks to weasely wording, the new error can later be reused for "extends"
errors.
Bug: T240858
Change-Id: I506e4dcd1151671f5302ecd99581145d979d8124
This exception was introduced very late in the patch I38c9929. It
already caused trouble. This here is essentially a revert. It restores
the previous behavior where this edge-case was silently ignored. The
worst thing that can happen is that appendText() creates an incomplete
entry in the $this->refs array, which will be rendered at the end. The
user can see it then.
As of now we are not aware of a code path where this would even be
possible. Still this does make the code *more* robust by not making it
explode, but give the user something they can work with.
Bug: T243221
Change-Id: I2e2d29bbd557090981903fcc2ece8796fafa4aa4
These create bogus output, depending on the surrounding wikitext the
<ref> tag is used in. For example, this example wikitext:
* Example.<ref name="1">a</ref> More text.
… will be rendered with the "More text" sentence wrapped on the next
line, outside of the list. However, this does *not* happen in many of
the localizations, e.g. German, because many Tanslatewiki translators
did not copied the bogus \n. Why should they.
TL;DR: These newline characters either do nothing, or destroy the output.
In both cases the proper fix is to replace them with spaces.
Some of the test cases touched in this patch demonstrate the issue.
Change-Id: I395a40637a5293eda1f477963d252ce1a215f8b2
This resolves another TODO. Since this is an intentional limitation in
the design of the feature, I find it pretty signigicant to give it it's
own error message.
Note that the text does not need to be perfect, just good enough for now.
We will review all error messages later via T238188.
Bug: T242141
Change-Id: Id9c863061e855350320131e81f6702c8810736f4
… if possible. In most cases it's possible to use the real object, and
reach into it's private parts via TestingAccessWrapper. This is almost
the same as using a mock, but I feel it's much more "light-weight".
The main change is that there is no strict assertion any more for the
number of ReferenceStack::pushInvalidRef() calls. Before this was mixed
into the same array as the valid references, as elements set to "false".
I think the test is as valueable as before without this extra check. If
the rollback stack works or not is already covered by other tests.
Change-Id: I90213557b164b3e43233a3dc393ee3f3d3d556a9
"Conflicting" here includes the case where one of two <ref> with the
same name does not have an extends attribute. The first occurence of
a name specifies if a <ref> is a top-level or a sub-reference. This can
not be changed later.
This patch changes multiple existing test cases. I checked all of them
in detail and confirmed the behavior is fine. The error reporting is
better or at least equally good in all cases.
Bug: T242141
Change-Id: Iaec306eefe5b168d496990105e297ca044a5e721
Allow a ref with `name=""` for backwards-compatibility.
Partially reverts I07738cce2641026dfaa92ba263ed6f9834be0944
Bug: T242437
Change-Id: Iaed2d1c41be377a4961aff39838b0965f6c00616
The difference between the two is that isOK() only reports "fatals",
while isGood() also reports "warnings" and "errors". I believe we
*want* to report all of these the same way.
Change-Id: I3be832c5db7aba3c03bd2ad8cfbba42362c093fd
A fun edge case where `name=""` fools both validation branches after
a references rollback, and triggered a LogicException. Stop these
freak refs.
Bug: T242437
Change-Id: I07738cce2641026dfaa92ba263ed6f9834be0944
It's possible to nest <references> by using tricky constructs like the
{{#tag function, and this breaks our rollback logic. Try to show normal
output, otherwise show an error.
Includes regression tests.
Bug: T242437
Change-Id: I33e497cdf8508ce7ccb7f0f315c00af5eee47d0e
Each of these TODOs is something that needs to be fixed or implemented,
so it's helpful to map them to tasks.
Change-Id: I807208392d8a609d7f3b371dc3560a48f3578092
* Always have an empty line between @param and @return to improve
readability as well as consistency within this codebase (before, both
styles have been used).
* Flip parameter order in validateRefInReferences() for consistency with
the rest of the code.
* In Cite::guardedRef() the Parser was now the 1st parameter. I changed
all related functions the same way to make the code less surprising.
* Same in CiteUnitTest. This is really just the @dataProvider. But I feel
it's still helpful to have the arguments in the same order everywhere, if
possible.
* Add a few strict type hints.
* It seems the preferred style for PHP7 return types is `… ) : string {`
with a space before the `:`. There is currently no PHPCS sniff for this.
However, I think this codebase should be consistent, one way or the other.
Change-Id: I91d232be727afd26ff20526ab4ef63aa5ba6bacf
The rollback feature was not able to properly restore a __placeholder__.
That's why a specific use case was behaving different. This already
worked just fine:
<ref extends="a">…</ref>
<references>
<ref name="a">…</ref>
</references>
But this didn't, even if it is the exact same from the users
perspective:
<ref extends="a">…</ref>
{{#tag:references|
<ref name="a">…</ref>
}}
Bug: T239810
Change-Id: I163a1bffb9450a9e7f776e32e66fb08d0452cdb9
Note this leaves *another* bug behind. When a <ref> is properly reused
by name="…", and the content is fine (either missing or identical),
possibly conflicting extends="…" attributes are currently entirely
ignored. However, this is already much better than what happened before.
Bug: T242110
Change-Id: Id808ce31c8036cc290f68bb3e8c5a7b12f4f44cf
This is an extremely relevant use case, but we never had a test for
this:
Some text.<ref extends="book">Page 2</ref>
<references>
<ref name="book">Title of the book</ref>
</references>
What this means: There is no reference in the text that points to the
book as a whole, only references that point to individual pages. The
base <ref> is not used in the text.
This is already properly rendered. There is no "jump back to the text"
link. However, this fails when <references> is wrapped in {{#tag:…}}.
Bug: T239810
Change-Id: Id22db0238266a4fd6131d1a10eb6bf6227552c19
I tried to run these tests with a very old version of this code base
(from 2018) to confirm this is the correct behavior.
Bug: T241303
Change-Id: Id97d016b199458aa178ca732282e9c0e91e291a4
One of the most significant changes is when I noticed that the $group
can never be null. We set it to DEFAULT_GROUP before. That's an empty
string.
I'm not very happy with the two @phan-suppress-next-line. Is there a
better way to fix these lines?
Change-Id: I33c1681e2f3857cb6701da71f4ed8893caff4d1e
I hope this is more readable. This patch does two things: It uses
array keys to name all elements in the data provider. (Note these
array keys don't actually do anything, PHPUnit ignores them.) And this
patch merges two parameters into a single $expectedResult.
Change-Id: Ib7adc32bf8bfd523735591d35d0bcabd3b853cfc
Since I3db5175 the ParserCloned hook handler does not rely on cloning
the Cite object any more. There is no cloning any more. This is dead
code and we could remove it. Just to be sure I propose to keep the
method, but let it throw an exception.
Bug: T240248
Change-Id: I2057ea652ca25f4c7031c28a6e713671738f5e22
These should be impossible conditions, we don't want to continue with
processing.
I hate this patch, it's a temporary workaround until someone rewrites
or replaces the rollback logic, for example with a two-pass parse.
Change-Id: I6a1327e397d4272fa412c3f290c2107d867d2854
I hope this patch is not to horrifying and can be reviewed. It's
possible to split this into a sequence of smaller patches. Please
tell me.
Change-Id: I4797fcd5612fcffb0df6c29ff575dd05f278bd4d
The main benefit is this nifty call: `$this->rollbackRef( ...$call )`
To make this possible, the minimal change I needed to do was to move
the two $argv and $text arguments to the end.
I also tried to order all other arguments as good as I could: Required
first, optional later. Group and name together. Name and extends
together.
All this is private implementation and should not affect anything.
Change-Id: I7af7636c465769aa53122eb40d964eabdd1289ba
I feel this is a little better than before. It looks like we never need
to *replace* a text that existed before.
This depends on I4a156aa which fixes one of the last remaining trimming
issues. Outside of <references>, a <ref> </ref> with no other content
but some whitespace was already forbidden. But not inside of <references>.
This is relevant for appendText(). It should not be called with null, but
was because of the inconsistent behavior.
Change-Id: I38c9929f2fa6e69482e45919e2f8dbf823cb1c8b
Note that this patch changes behavior, an invalid "dir" will result in
a cite reference at the point where the <ref> is declared rather than
in the references section. This is consistent with other errors.
Bug: T15673
Change-Id: Id10db40aa0b391f2f1d9274aa09d22a7278d65e3
The name of the base class in tests is guaranteed to only occur a
single time in a file. There is not much value in making it relative,
and requiring it to appear in the use section. Especially because it
is in the root namespace.
This reflects what I once encoded in the sniff
https://github.com/wmde/WikibaseCodeSniffer/blob/master/Wikibase/Sniffs/Namespaces/FullQualifiedClassNameSniff.php
I wish we could pick this rule and use it in our codebases. But it
seems it is to specific and can't be applied on all codebases, hence
it can't become part of the upstream MediaWiki rule set. At least not
at the moment.
Change-Id: I77c2490c565b7a468c5c944301fc684d20206ec4
This makes one of the last remaining edge-cases about non-empty, but
non-visible content (a <ref> that only contains whitespace) behave
identical to all other places. We already reported it as being empty
everywhere else, except inside of <references>.
Note that the test cases look like they are reporting the same errors
twice. But this is not the case:
The first set of errors is about <ref name="…"> inside of <references>
not having visible content. This should always be reported, even if the
<ref> got content from somewhere else on the page.
The second set of errors is when a <ref name="…"> *never* got any
content.
This patch will slightly increase the numbers of errors reported.
Change-Id: I4a156aa9e466f735d92fe0ba5cc0678ec8bbdd50
* 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
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
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
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
* 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
We are *so* close to 90%.
This patch should raise the coverage for the CiteDataModule to 100%.
I'm also adding a pure unit test for the clone() behavior. Note the
later is already covered by the CiteDbTest.
Question: Do we want the CiteDbTest to @cover anything?
Change-Id: I40763d01e18991f509bc30b6655aa57b23412fd9
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
Tickle very particular edge case in which a recursive parse corrupts
the $parser->extCite object.
Bug: T240248
Change-Id: I70d100e88fa72825194ed9c477b030bbf0b6b486
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
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
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
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
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
These edge cases are handled correctly already, I just forgot to
remove the TODOs when updating test content.
Note that there's only one TODO left, and it's to forbid a feature which
actually works!
Change-Id: I0d3a1f55f0ce943b0d034dda40e3779fbf241fe4
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
