There is currently no test coverage for recursively parsing the
contents of a <ref>…</ref> together with an incomplete follow="…".
This is critical because that's an entirely separate, special code
path (the one that creates a <p> instead of an <li>). Without this
test we could return unparsed wikitext and never notice.
I discovered this while playing with I0b0e358.
Bug: T245549
Change-Id: Ie65c6bf6bf75db26e0fff733c93cfa28ee7bd228
This does the exact same as the previously used generic stdClass
object, just strictly typed. Turned out to be surprisingly
straightforward, as proven by the small size of this patch.
I'm intentionally not adding anything new in this patch. For
example, the new class is perfect to write longer documentation
for every field. But this is for a later patch.
Change-Id: Ibf696f6b5ef1bfdbe846b571fb7e9ded96693351
The ext.cite.referencePreviews module will transparently replace the
ext.popups.referencePreviews module after this patch. Configuration
stays in Popups for now, we can migrate it in later work.
CSS classes may be renamed in the future but this will be handled
separately since it could be a breaking change for on-wiki
customizations.
A lot of fancy footwork happens in this patch to emulate a soft
dependency on Popups. This mechanism doesn't exist explicitly in
either ResourceLoader or QUnit, so lots of workarounds are used, to
conditionally load the module and to dynamically skip dependent tests.
renderer.test.js is fully skipped for now, but can be wired up in
later work.
Bug: T355194
Change-Id: I0dc47abb59a40d4e41e7dda0eb7b415a2e1ae508
There is much more to test, but it's a start.
Intentionally build as pure unit tests to make them as fast as
possible.
Bug: T354215
Bug: T358652
Change-Id: Iae1a8086b8f2b9e5b11e0117bd3f19fdaa087df0
The first two files have been added to the root modules/ directory
via I487095d in 2015. No problem.
Many, many more files have been added via I000b453 in 2022. It's
really hard to tell what is what since then.
I'm not absolutely sure what the naming convention for this folder
should be. Could as well be "localized-styles/" or just "Parsoid/".
Bug: T156350
Change-Id: Ibcf8c7a6db5400ed8a9811244a070e03ff372a39
The information read from the …cite-tool-definition.json files is
effectively user input, even if only interface administrators can
edit it. Usually we carefully validate user input. But as of now
this code starts failing with all kinds of uncatched errors.
* An entry with no name, an empty name, or a name that's not a
string will cause all kinds of undefined behavior.
* An entry with an empty title results in an invisible button.
* A missing message results in a technical <…> placeholder, even if
the name is usually a sensible fallback.
Note that hard-coding titles as plain text strings in the ….json
file was already possible.
Change-Id: Iddcedbe859e86ac4c3f79a53d36237daff86c0db
The message was part of the original patch that introduced the group
feature in 2009, see https://phabricator.wikimedia.org/rECIT75004e33.
Notice how there was never a test scenario for this message. A test
was added in 2020 via I07738cc.
The message appears only in a rare edge-case when a group is entirely
unused in the text, and only when the group is not empty. The shortest
possible example is:
<references group=g>
<ref group=g name=a>a</ref>
</references>
Just adding something unrelated like `<ref group=g>x</ref>` to the
text changes the error message. Now the group is "used". But this
notion is confusing to begin with. References can be part of a group,
and we can use references, but we can't use groups as if they are a
separate entity.
A better error message already exists.
Notice how this special error message doesn't appear anywhere in the
Parsoid code path. That was already using the other, more generic
error message.
Bug: T269531
Change-Id: I63f663d76e45e6c3d664f145d9a564ee00ff53cd
This is about the error message that currently says:
»Cite error: <ref> tag with name "a" defined in <references> has
group attribute "" which does not appear in prior text.«
This is a special error message that appears only when a group name
does not appear anywhere in the text. In all other cases a simpler
error message is shown:
»Cite error: <ref> tag with name "a" defined in <references> is
not used in prior text.«
While the first error message is not wrong in the edge-case
described in T269531, it's very confusing for a multitude of
reasons. For example:
* There is no group attribute in the wikitext.
* Just adding something completely unrelated like `<ref>x</ref>` to
the text shows the other error message.
The reason for this behavior is that the assumed default is an empty
`group=""`. The error message changes the moment any other <ref> in
the same group appears in the text vs. when the group is entirely
unused.
We can probably remove this error message entirely, but should at
least not use it when there is no group.
Notice how the Parsoid code path was already using the other error
message.
Bug: T269531
Change-Id: Ifa2e97254f4cda72233a057d8760fb1116143552
I always found the name a little ambiguous. The fact that it outputs
an actual HTML list and not just some "references" – whatever that
means – is relevant, in my opinion.
Change-Id: I0d169455c8d2b42d62da4dccb8376c09fb6902bc
… as well as "cite_warning". Both are extremely trivial and don't
really do anything by default. All they do is to add the prefix
"Cite error:" or "Cite warning:" to all error messages.
This patch will make it possible to disable both messages by
default, i.e. replace their default in en.json with "-" without
breaking anything. That's part of the plan outlined in T353695.
Local on-wiki overrides will continue to work.
Bug: T353695
Change-Id: I374800d0d0b837cd17ed3a1fdde20b70325b06de
This was slightly overengineered ever since I4b1f890 and slowly became
more and more complicated over time, notably when withConsecutive was
replaced in Icb951b4. Turns out this was never really needed. It's
impossible to get more than one tracking category from this code path.
While we might add more tracking categories later that will most
probably not happen in this code path.
Change-Id: Ie32d17bac8d3518c985b18f83a846c3fb2bd053f
* This is to worka round some confusing html2html failures in CI
for these tests (that are not reproducible locally for me).
Change-Id: I07725155ef5e04eb4346a90c34cbacbd70e88ea6
These tests pass today because Parsoid is providing an
alternative implementation of Cite, but that means this
test case isn't actually testing the code in this repo.
Bug: T354215
Change-Id: I42521026bab36035ae5eded7c05716234a5a29ea
General improvements
Ensure proper module loading in Cypress tests to prevent failures by waiting for specific modules to be loaded
Delete the selenium test suite.
Bug: T353436
Change-Id: Ieb71b122d3c6513f1a15d6574967e2bb9c21a393
Intentionally no other change is made (yet). This is for a later,
separate patch.
Intentionally not touching the huge list of per-language
ext.cite.style.*.css files for the moment. Again, I would prefer to
do this in a separate patch.
Change-Id: I4e392c7bd1c69849a6c7946676a64c749ddbcd60
This commit also moves certain parser tests involving <ref> from
the Parsoid repo to citeParserTests.txt in this repo.
Bug: T354215
Change-Id: Ie5b211d2af01a56684473723c68a9ab2775542e3
Some interesting stuff is happening, seems to have revealed bugs:
* Rolled-back warnings are still present on the ref
* Subref reuse numbering starts at 0 instead of 1, and formatting is cringe.
But subref rollback does seem to work!
Change-Id: If6321b34d27370553ba85e63dd1e2ae6a3b7c099
Such a message shouldn't exist, and doesn't:
https://global-search.toolforge.org/?q=.®ex=1&namespaces=8&title=Cite+link+label+group-
Additional notes:
* Rename the method to make it more obvious that it's not a cheap
getter, but doing something slightly more expensive.
* Use more appropriate array_key_exists to check if a cache entry
already exists.
* Also add a bit more documentation.
Bug: T297430
Bug: T353227
Change-Id: Ia5827bbf6fd700b87a749aac17320796428f0688
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 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
Stop relying on the magic number distinction between "count" = 0 and -1,
by explicitly testing the "name" field instead.
Bug: T353227
Change-Id: I9dce16b01814e19f508d45b927de570049f0e0f5
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
These fields get automatic values during normal operation, but we
should make this explicit in tests which meddle with internals. This
seems to add some clarity, and helps prepare for encapsulation.
Bug: T353451
Change-Id: I8b012a270f16139671f77ea04645d627b2fba87d
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 is another improvement after I7390b68. Status objects are made
to keep track of multiple errors. The only difference is: The merge
method skips duplicates when the message and all parameters are
identical. This causes a minor user-facing change. One of the
shortest possible examples is:
<references>
<ref />
<ref />
</references>
This showed two identical, indistinguishable error messages before,
but will only show one now. We argue this is fine. The duplicates
are confusing and of (almost) no value to the user. In case the
information is relevant the correct solution is to make the error
messages distinguishable, or introduce a message like "multiple
<ref> tags defined in <references> have the same error". This is
something for a later patch, if needed.
Bug: T353266
Change-Id: I444105462ed24d5ba37b057622b4dc847b40f8d8
Testing internal methods is brittle. This code path is already
covered by parser test "Valid follow="…" after it's parent"
Bug: T353451
Change-Id: I3b7a4b9962de1f25a7b57f82d80813219d633594
Same as Icfa8215 where we removed the …_suffix messages.
This patch is not blocked on anything according to CodeSearch:
https://codesearch.wmcloud.org/search/?q=cite_references%3F_link_prefix
According to GlobalSearch there are 2 usages we need to talk about:
https://global-search.toolforge.org/?q=.®ex=1&namespaces=8&title=Cite.references%3F.link.prefix.*
zh.wiktionary replaces "cite_ref-" with "_ref-", and "cite_note-"
with "_note-", i.e. they did nothing but remove the word "cite". This
happened in 2006, with no explanation.
ka.wikibooks and ka.wikiquote replace "cite_note-" with "_შენიშვნა-",
which translates back to "_note-". One user did this in 2007,
16 seconds apart.
It appears like both are attempts to localize what can be localized,
no matter if it's really necessary or not.
https://zh.wiktionary.org/wiki/Special:Contributions/Shibo77?offset=20060510https://ka.wikiquote.org/wiki/Special:Contributions/Trulala?offset=20070219
Note how one user experimented with an "a" in some of the edits to
see what effect the change might have, to imediatelly revert it.
The modifications don't really have an effect on anything, except on
the anchors in the resulting <a href="#_ref-5"> and <sup id="_ref-5">
HTML. It might also be briefly visible in the browser's address bar
when such a link is clicked. We can only assume the two users did this
to make the URL appear shorter (?). A discussion apparently never
happened. Bot users are inactive.
Both pieces of HTML are generated in the Cite code. Removing the
messages will change all places the same time. All links will
continue to work. The only possible effect is that hard-coded
weblinks to an individual reference will link to the top of the
article instead. But:
a) This is extremely unlikely to happen. There is no reason to link
to a reference from outside of the article.
b) Such links are not guaranteed to work anyway as they can break
for a multitude of other reasons, e.g. the <ref> being renamed,
removed, or replaced.
c) Even if such a link breaks, it still links to the correct article.
There is also no on-wiki code on zh.wiktionary that would do anything
with the shortened prefix:
https://zh.wiktionary.org/w/index.php?search=insource%3A%2F_%28ref%7Cnote%29-%2F&title=Special%3A%E6%90%9C%E7%B4%A2&profile=advanced&fulltext=1&ns2=1&ns4=1&ns8=1&ns10=1&ns12=1&ns828=1&ns2300=1
I argue this is safe to remove, even without contacting the mentioned
communities first.
Bug: T321217
Change-Id: I160a119710dc35679dbdc2f39ddf453dbd5a5dfa
This fixes a minor issue introduced in I294b59f. Two identical
dir="…" with different capitalizations should not be reported as an
error.
Turns out the implementation in the Cite extension doesn't care
about this capitalization at all. That's why I suggest to do the
normalization as early as possible. This is slightly different in
the Parsoid implementation.
Bug: T202593
Change-Id: I96b4a281d6020d61d1f36ec027cf833bbb244f03
* Since Cite development happens in two repos (here and the Parsoid
repo), integrated tests ensures that changes don't fall too far
out of sync.
CI runs Parsoid-integrated-mode tests in extensions repo with the
vendor-released Parsoid.
Parsoid CI runs Parsoid-standalone-mode tests in the Parsoid repo
which also has a copy of the citeParserTests.txt file found here.
But, that CI run uses the Parsoid patch itself.
This difference makes for unnecessrily laborious test syncing
while making changes to the two repos. It is manageable for one-off
changes but when making lots of updates that changes tests a lot,
this quickly becomes painful.
* For now, we can break this coupling temporarily by disabling
Parsoid-integrated-mode test runs. This simplifies the test syncing
by letting patches in Cite repo to be merged in a chain and then
doing a single test sync to the Parsoid repo (otherwise, Parsoid's
CI will be broken since the html/php sections in Parsoid's cite
test copy will be out of date).
* Filed T354215 to move Parsoid's Cite implemntation to this repo
which eliminates this complexity altogether.
Change-Id: Id5727381b0e23058d098180c308797b2555ad02f
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 now aligns with Parsoid commit 0fab92ba453d424aedeadaaa9e1514c42bbd94d1
* Disabled the newly added tests because that Parsoid fixes for the
tests haven't been released to vendor to let CI pass these tests.
* Re-enabled a previously disabled test.
Change-Id: I4ab87d2d486b7a1fef652c50c4f1e79ddfe83ce6
This reverts commit b163add15b.
Reason for revert: This was my mistake. I forgot that reverting this
would break Parsoid CI once the Parsoid Cite patch merged. So, I have to
wait till the Parsoid Cite change is released to vendor before I sync
the test change here.
Change-Id: Icaecee1e56907980681aae01be377b6906bd93a6
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
For example, use convenient upstream methods, and generally make the
test setup a bit more readable.
Bug: T353227
Change-Id: Ifab71041fcc3f804315793ca7b783f84829c7a0f
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
I wanted to make this a unit test but it turns out the
Sanitizer::safeEncodeAttribute() calls currently make this
impossible.
Bug: T353269
Change-Id: I5266e7b8b67db1c812dc9e4675d0c079ab1f9a40
This patch only moves existing code around without changing any
behavior. What I basically did was merging the old "guardedReferences"
method into "references", and then splitting the resulting code in
other ways. Now we see a few other concepts emerging. But the idea
something would be "guarded" (how?) is gone.
The most critical detail in this patch are the new method names, and
how the code is split. The names should tell a story, and the methods
should do exactly what the name says. Suggestions?
Bug: T353266
Change-Id: I8b7921ce24487e9657e4193ea6a2e3e7d7b0b1c3
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
> We lose useful coverage and spend valuable time keeping these tags
> accurate through refactors (or worse, forget to do so).
>
> I am not disabling the "only track coverage of specified subject"
> benefits, nor am I claiming coverage in in classes outside the
> subject under test.
>
> Tracking tiny per-method details wastes time in keeping tags
> in sync during refactors, and time to realize (and fix) when people
> inevitably don't keep them in sync, and time lost in finding
> uncovered code to write tests for only to realize it was already
> covered but "not yet claimed".
https://gerrit.wikimedia.org/r/q/owner:Krinkle+is:merged+message:%2522Widen%2522
Change-Id: Iafa241210b81ba1cbfee74e3920fb044c86d09fc
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 played around with a few options (see patchset 1) but ended
introducing new terminology:
* "Backlink" describes the ↑ button down in the list of <references>
that jumps back up into the article. The code was already using
"backlink" in some places.
* "Backlink target" is the id="…" attribute up there, visible as the
typical [1] in the article.
* I use "jump" to describe the idea that clicking the [1] jumps down
to the full reference.
* "Jump target" is the id="…" down there in the list of <references>.
* "Jump link" is the same id, but encoded to be used as the href="…"
attribute when clicking the [1].
I hope this makes sense. Suggestions welcome.
Another benefit is that "normalization" is really only normalization
now, not any URL and/or HTML encoding.
Bug: T298278
Change-Id: I5a64ac43aef895110b61df65b27f683b131886fb
This moves the actual parsing down to be done much later in the
process. This won't make any difference in production but makes it
easier to refactor the code further.
Note I tried to use a StatusValue object but couldn't because it
merges seemingly identical messages, while the plain array is fine
with containing duplicates. There is one parser test that covers
this. While we could change this it needs discussion and most
probably a PM decision.
Change-Id: I7390b688a33dace95753470a927bbe4de43ea03a
The "parser marker" placeholders are case-sensitive, e.g. for a tag
that's written like <rEf> the placeholder will also say …-rEf-…. This
was really just a mistake.
The error is as old as this code is. Added in commit 75004e33 in
2009.
Note we shouldn't use /i at the end because the marker itself should
not be case-insensitive. Only the tag name.
Instead of adding more (slow) test cases I update two that are
exactly about this part of Cite (nested tags) anyway.
Bug: T64335
Change-Id: I44c7a42a0da682a1082952fd1af817bf7d45378c
Two problems:
1. Manipulating globals directly affects all following tests. They
are not independent from each other. This problem can be seen in
CiteTest.
2. Some test cases in testValidateRef don't test what you think.
For example, the test for a conflicting "extends" + "follow" was not
failing because of the conflict but because "extends" was disabled
and disallowed.
Change-Id: Iaa4e1f3f3222155d59984e577cba3f0b8dec40c3
