isset() should only be used to suppress errors, not for null check.
When the variable is always defined, there is no need to use isset.
Found by a new phan plugin (2efea9f989)
https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP#isset
Change-Id: I2eee98fdcb21192108183c431b10e0072b951f72
This test was obscured by testing for a field on the parent, but that
would exist if and only if the parent also existed. Clarify the
guard condition and introduce a named local variable for the parent.
Change-Id: I03079f45cf5ba00d54642c89ac4232a944b2f353
This encapsulation gives us field name, type validation and code
documentation.
This patch only affects ReferenceStack and continues to return
approximately the same array outputs to callers. Some additional
information is included and the placeholder column has a new name.
Bug: T353451
Change-Id: I405fe7ac241f6991fd4c526bfbb58fbc34f2e147
The placeholder field will only be set if the ref exists, so we can
put these in a more logical order.
Change-Id: I2ddfb501fcc3aca936bb45c0d40e4f68c5d2b192
The previous patch deprecated the last conditional depending on magic
meanings of 0 and -1, so now we're free to let "count" take on a more
natural meaning: the number of times a footnote mark appears in
article text.
Includes a small hack to avoid changing parser output, by
artificially decrementing the count by one during rendering. The
hack can be removed and test output updated in a separate patch.
Bug: T353227
Change-Id: I6f76c50357b274ff97321533e52f435798048268
These can be hard to read so this patch introduces named, temporary
variables.
PHP reference assignment is helpful here, and has the nice property
of responding correctly to `isset` as if it were called on the
referenced variable. However, we're prevented from using this trick
in more places in the code because of an unfortunate side-effect that
PHP will store `null` under the referenced array key. In some cases
(the ones here), this is harmless because we always test using
`isset` and null behaves the same as an unset value. In other cases
such as arrays that are iterated over, the spurious key and null
value would be more of a nuisance.
Bug: T353227
Change-Id: Ie43592a2f10677ba19842e92fa29eb4bf3be240c
Encapsulate all information about a ref inside of the internal
structure, rather than relying on the container to be organized by
group.
Bug: T353451
Change-Id: I4c91e8089638b7655bf120402a4a5fcbd1b35452
In this case, there was never a ref with this name in the article so
no backlinks should be rendered.
TODO:
* test case with empty parent backlink and LDR parent
Bug: T353451
Change-Id: I8a7abd05a48ce83da3beb92b15e894d53252bd33
This classifies as a "warning" because we still show everything,
just with an error message appended.
Disabling the Parsoid tests right away hopefully makes it easier to
do the same change in Parsoid.
Bug: T202593
Depends-On: If14acd1070617ca8c4d15be6b1759bd47ead4926
Change-Id: I294b59f989f553932b40d08308906dd72d92d2cd
This moves one more error situation into the stack class, together
with other error situations that are already there.
Bug: T353266
Change-Id: Icf169650f67f64e6d29d175c3b47cf558b8de3d4
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
This is a concept that's only relevant when a sub-reference (formerly
known as BookReferencing) appears before the parent reference it
belongs to. Let the name reflect this.
Bug: T353227
Change-Id: Iabf259e72942ea70cb1cc1e0ca5a5d8cf15d7225
This removes almost 200 lines from the main class.
This patch intentionally doesn't make any changes to the code but
only moves it around. Further improvements are for later patches.
Bug: T353269
Change-Id: Ic73f1b7458b3f7b7b89806a88a1111161e3cf094
The main benefit is that the two lines that set and reset
$this->inReferencesGroup are now next to each other. More can be
done in later patches.
Bug: T353266
Change-Id: Ib3f40c40e0b1854f8e5a32af600f28931fffdb8c
I tried many things, but wasn't able to reproduce the error
described in T283755. What probably happens goes like this:
* Somehow $this->refs[$group] is initialized, but
$this->groupRefSequence[$group] is not.
* There are not many places in the code where this can
happen. There are a few suspicious lines in rollbackRef(),
but they are all guarded. The only problematic place is in
appendText().
* This problematic line is only called for <ref> in
<references>.
* Somehow a <ref> is valid enough to make it to appendText(),
but not valid enough to make it to pushRef().
* The next time another <ref> is added to the same group, it
appears like the group already exists ($this->refs[$group]
is set), but $this->groupRefSequence[$group] is missing.
I was unable to find a wikitext example that would behave like
this.
This patch just makes sure the initialization is done but
doesn't care why it was missing. The following code is fine
with an existing ref that contains nothing but text (which is
how appendText() leaves it behind).
Bug: T283755
Change-Id: I36ac56ef6ed98676a3e8f430a796826351a5f4e9
This patch also adds a test case that was missing before. If a
follow="…" is followed by another, normal <ref>, the internal key
(a.k.a. $this->refSequence) is not incremented. This was the case
before, just not covered by any test.
Change-Id: I102d1e67a6918017acc7e4a4663b08c828d101a6
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
