While working on the patch I4303642 I was worried about the line
array_pop( $this->refCallStack )
in the rollback code. Since the patch changed the position of follow
elements in the stack, an array_pop() would pop different elements.
It turns out this is impossible. Rollbacks are only done for <ref>
elements inside a <references> tag, immediatelly after reaching the
closing </references>. It's impossible to use follow="…" inside
<references>. It will not be added to the stack, and therefore not
rolled back.
Even if the edge case would be possible, the *old* code that placed
follow elements on the *other* side of the stack would have been
wrong then.
The test cases in this patch try to hit this edge case, and are
expected to not be able to do so.
Change-Id: I4380bf443db17c6214dbfa2cbda62b46db04258a
Previously the reflist was added at the end of the last line of text,
which messes up paragraph wrapping (as seen in many test cases), and
generated invalid HTML when the last line was a list item (T148701).
Bug: T148701
Change-Id: Ifc873fc913e717026d80d54b570c594d1073fb42
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
The isModuleRegistered() method was introduced a few years ago,
when the load order in ResourceLoader was undergoing a change.
It used to be that hooks like were run first to register modules, and then
wgResourceModules was registered afterwards. This was reversed to disallow
mutating the config at run-time from foreign modules and to allow better
caching and error detection.
It's been several years since then, so this redundant check is no longer
needed. ServiceWiring.php in MW core for ResourceLoader always processes
config and extension.json first before this hook is called.
Bug: T247265
Change-Id: I466f1fa70b8f0e9fe5e8e8df90bb0001b3329b87
Html5 fragment mode now bans whitespace per html5 spec
See Ie2b7c9429691e2c491c3359d5b400d8f078aa789
Change-Id: Ie6fa40798f06a358f6082110b4d8cc0028c80321
This reverts parts of the revert I3bee35f, which reverted a3d312c8.
I believe it's helpful to keep these test cases just to document how
the code currently behaves. I removed all TODO because we don't know
if and how we want to touch this again.
Bug: T240858
Change-Id: Ib91acfcb7292e5c03ce9cc4d7be782085e10aa27
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
