I noticed a possible issue related to the $this->refSequence counter
in the patch Ida9612d. Some of these counters might get messes up, but
there was never a test that checked what will happen to the *next*
reference then.
I checked the test cases in this patch with a very old version of the
codebase.
Change-Id: If6e56f727dce5d0e5e38e048e602437597248a42
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
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
These exist two times, one time in the unit/ folder as a unit test, and
another time in the parent folder as an integration test. This confused
me already several times.
Change-Id: I147b8af8a7edba2582496468b4878faecc6d8110
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
That was annoying me. Since we're passing a bare list, alphabetical
order helps make the code and tests readable.
Change-Id: I6384094e429e0e2a6fa810fdc28ae0643a0ccf7c
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
I realized especially the method name html() was wrong. It does not
return HTML. What it returns is still wikitext and must still be parsed.
It only applies some early steps of the parsing process, e.g. expanding
extension <tags>.
Change-Id: I2c403a77eef843940f34f0933e4bfe58e6200ce5
* This fixes the refArg() function. If there is nothing wrong with the
follow="…" attribute, it should not return null.
* However, *everything* is false if an unknown error (e.g. an unknown
attribute) occurs.
* A trivial check for `if ( $follow )` is fine because all keys are
guaranteed to not be the string "0".
Change-Id: Ia4e37781e01db1ee6615ffc30bb68e47023c6634
One of the test cases was duplicated, but a lot of the possible code
paths never had tests, including the happy code path!
I found this issue while trying to rework some of the more confusing
loops in this codebase. These changes are still part of this patch. All
loops still do the same as before, but are (I hope) more readable now.
Bug: T238187
Change-Id: I85baeadd9b149025a14c7522bcc4182339c66972
… and make the error message for bad dir="…" shorter and more to the
point.
Now I understand why the error reporting was not done when $text was
empty: the error was actually appended to $text, which messes with
everything else that also works with the $text variable! This even
includes the API. This error message was exposed via the API. That was
certainly a bug.
With this patch, all error checking for the dir="…" attribute is now
done way down, when rendering the <references> section.
Note this also fixes a bug where the dir="…" was *not* rendered when
previewing a section.
Change-Id: I4ab0cb510973ed879c606bfaa394aacc91129854
This fixes a whole bunch of inconsistencies:
* The dir attribute is now trimmed, as most others already are. This is
an actual user-facing change.
* The internal representation is now false in case the value was invalid,
not an empty string any more.
* Null means the attribute was not present. This is now always used,
even in the return values that are meant to represent an error state. No
existing behavior changes.
* The internal representation does not contain an HTML snippet any more,
but the raw value "ltr" or "rtl", or null. Note this might influence the
API, because the API actually exposes the internal representation.
However, we are pretty sure the API is not used anywhere. Even if,
exposing HTML code was most certainly an unwanted and unexpected effect
of the patch that introduced the dir attribute. This does make this a
bugfix, I would argue.
Change-Id: Ic385d9ab36fa0545c374d3d63063028ae4e449d4
This patch does intentionally not touch any file name. Some of the
file names are a little weird now, e.g. \Cite\Cite. These can more
easily be renamed in later patches.
I used https://codesearch.wmflabs.org/search/?q=new%20Cite%5C( and it
looks like this code is not used anywhere else.
Change-Id: I5f93a224e9cacf45b7a0d68c216a78723364dd96
The use case we care about is this:
<ref extends="some_book"> </ref>
It doesn't make sense that works, but the following doesn't:
<ref extends="some_book"></ref>
We decided that both need to behave the same.
For consistency this patch is applying the same change to all references,
no matter if they use the extends attribute or not. This is an actual
change and might make existing wikitext render differently. However, I
would like to argue that all wikitext that was using this was broken. The
effect of a <ref> </ref> with some whitespace is that the <references>
section at the end of the article will contain – well – an empty footnote.
Bug: T237241
Change-Id: Iaee35583eabcb416b0a06849b89ebbfb0fb7fef9
Note this codebase appears to be dual-licensed. Some files mention MIT,
but extension.json and some other files mention GPL.
Since WMDE typically uses GPL, I will continue to mark the files we
created as such.
Change-Id: I126da10f7fb13a6d4c99e96e72d024b2e5ecee06
The main motivation here is to cover the fallback code that was moved
in I20c814d. At some point we might touch this code again.
Bug: T238194
Change-Id: I0ab8a34b09790f42b10376eb3730c3b3c4ef53d2
1. Most existing CiteTests can be unit tests. They run so much faster
this way.
2. I modified some test cases to cover all trim() in the code.
3. The strict type hint in CiteHooks is removed because the parameter
is not used. Having a hard type hint for what is effectively dead code
makes the code more brittle for changes done outside of this codebase.
Change-Id: I1bff1d6e02d9ef17d5e6b66aeec3ee42bba99cf4
This fixes a series of issues:
* There is nothing about a "frame" in the Cite class any more.
* There is no addModules() call in the Cite class any more.
Change-Id: I20c814d46c26825c5c07eab0a5586de3a531eee7
To be honest I don't get why this lazy registration was done in the
first place. None of the 4 other hooks should ever be called before
the ParserFirstCallInit hook got called.
Also, under which circumstances can the ParserFirstCallInit hook be
called more than once?
Both scenarios would be wrong, as far as I'm concerned. Either I'm
missing something, or this code can indeed be simplified. Maybe it was
something to make it more compatible with older MediaWiki versions?
The only reason I can think of is: in all situations that do not
involve a parser, having the 4 extra hooks registered is pointless.
Does this waste space and/or runtime in the $wgHooks registry?
Change-Id: I5ef1495f4ce7bce940fa5f8e700af3d2c4851a01
Can use a shortcut where we pass the expected value directly. Verified
that we're still asserting equality.
Change-Id: I63512488c50e599df23d5dae2a5064218e311e90
