Check out how this gets rid of so many "to do" as well as
"deprecated" comments.
Next qustion: The elements in the stack become more and more
complicated. It's probably worth converting them from arrays into
first-class objects. But this is for another patch.
Bug: T353266
Change-Id: If14acd1070617ca8c4d15be6b1759bd47ead4926
Same arguments as in Iafa2412. The one reason to use more detailled
per-method @covers annotations is to avoid "accidental coverage"
where code is marked as being covered by tests that don't assert
anything that would be meaningful for this code. This is especially a
problem with older, bigger classes with lots of side effects.
But all the new classes we introduced over the years are small, with
predictable, local effects.
That's also why we keep the more detailled @covers annotations for
the original Cite class.
Bug: T353227
Bug: T353269
Change-Id: I69850f4d740d8ad5a7c2368b9068dc91e47cc797
Both Language and Parser are extremely complex classes with hundreds
of public methods. We really want to make sure we are not depending
on anything unexpected from these classes. If calls are made into
these classes we want to know exactly what is called.
Doing this also showed that some mocked methods are not even needed.
Change-Id: Icdfff6c07be78a47bf7cadb1813a72581a51272a
This is mostly because recent IDEs can understand createMock() quite
good. We usually don't add such hints every time we use createMock().
We would have a million of them. ;-)
Change-Id: If9e37807a6945c4408d374fc97664cd636020ffd
This removes a few tiny pieces of code, and a large chunk related to
incomplete follow="…" attributes (see T240858). It turns out we don't
need to insert elements at the top of the ReferenceStack::$refs
array, because this array is reordered anyway in
ReferencesFormatter::formatRefsList()!
Incomplete follow refs don't have a number, and are ordered to the top
because of this, as before. This doesn't change with this patch.
Change-Id: I43036420be22feb8f0f287d9ccee2afd317df2a9
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
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
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
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
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
* 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
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
If `extends` is encountered before the parent ref, we reserve the
sequence number and leave a placeholder to record the link between
ref name and number. This is necessary to render a list like,
"[1] [2.1] [2]", or to use subreferencing when the parent ref is
declared in the references tag.
When a placeholder is encountered during references section rendering,
it means that the parent was never declared.
Change-Id: I611cd1d73f775908926a803fae90d039ce122ab6
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
This was carrying the entire footnote marker, but subreferences need
to extract just the first (group ref sequence) part. Storing number
and extendsIndex in two separate fields gives us more flexibility
during rendering, for example these might use two different symbol sets.
Change-Id: I75bd6644c336036f9e84ba91e1c35e05bc1ca7f3
Validation blocks (name==null && text==null), so it should not be a
test case. Give the text a non-null value.
Also adds a check for missing test data.
Change-Id: I0f02206e2221805f5a2f8eaa163ed237cfb8d777
Forked from Icd933fc983.
Bugs and unimplemented features are documented as TODOs in the parser test
fixtures.
Bug: T237241
Change-Id: I9427e025ea0bcf2fa24fd539a775429cc64767cc
Functional changes:
* hasGroup() will return false when a group exists, but is empty. This
is in line with what other methods like getGroups() already do.
Shouldn't have any effect on the existing code, but feels more clean
and consistent.
* getGroupRefs() won't fail any more when asked for an unknown group.
Tests:
* Add missing @covers for the constructor.
* Simplify test setup by always returning a spy. All tests need it
anyway.
* Cover 3 more methods.
Change-Id: Ie93e9af6258b757d842b30b0b059344733aad434
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
