Commit graph

34 commits

Author SHA1 Message Date
Thiemo Kreuz 816b1b0add Remove newline characters from all error messages
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
2020-01-24 12:29:14 +01:00
Thiemo Kreuz 51d55bb8de Introduce dedicated error message for nested <ref extends=…>
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
2020-01-23 15:00:26 +01:00
Adam Wight 8a58ed55dc Error when reusing <ref> with conflicting "extends" attributes
"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
2020-01-20 13:33:52 +00:00
Adam Wight 170484e933 Annotate TODOs with task number
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
2020-01-09 13:13:48 +01:00
jenkins-bot 0be582dc12 Merge "Report conflicting extends="…" with an error message" 2020-01-09 11:37:27 +00:00
jenkins-bot b9b6905171 Merge "Fix incomplete undo/redo stack implementation" 2020-01-09 10:58:35 +00:00
Thiemo Kreuz 04fbbbd3ca Report conflicting extends="…" with an error message
Bug: T242110
Change-Id: I04342b2c219981dfb9575ea58cfccf6c2ba1066c
2020-01-08 16:47:07 +00:00
Thiemo Kreuz d07110b790 Fix incomplete undo/redo stack implementation
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
2020-01-08 17:43:02 +01:00
Adam Wight b7c9dbb0d5 Remove invalid test case
Unnamed references are never merged.

Bug: T239788
Bug: T240459
Change-Id: I8dd3706c688108bf2e3c0e9b55f123084b325d16
2020-01-08 16:59:28 +01:00
jenkins-bot 861c4edba7 Merge "Test cases for extends pointing to the <references> section" 2020-01-08 10:42:14 +00:00
Thiemo Kreuz 6ddfd9983b Fix bad numbering when reusing sub-references
Note this leaves *another* bug behind. When a <ref> is properly reused
by name="…", and the content is fine (either missing or identical),
possibly conflicting extends="…" attributes are currently entirely
ignored. However, this is already much better than what happened before.

Bug: T242110
Change-Id: Id808ce31c8036cc290f68bb3e8c5a7b12f4f44cf
2020-01-07 16:34:05 +01:00
Thiemo Kreuz 5db90fb5a9 Test cases for extends pointing to the <references> section
This is an extremely relevant use case, but we never had a test for
this:

Some text.<ref extends="book">Page 2</ref>

<references>
  <ref name="book">Title of the book</ref>
</references>

What this means: There is no reference in the text that points to the
book as a whole, only references that point to individual pages. The
base <ref> is not used in the text.

This is already properly rendered. There is no "jump back to the text"
link. However, this fails when <references> is wrapped in {{#tag:…}}.

Bug: T239810
Change-Id: Id22db0238266a4fd6131d1a10eb6bf6227552c19
2020-01-07 12:43:18 +01:00
Thiemo Kreuz f86b5073fd Add parser tests for reused extended <ref> before defined
Bug: T240424
Change-Id: I945c2e12cfa3ff851380a1ff4491c8af076f523a
2019-12-11 16:30:17 +01:00
Thiemo Kreuz 193b840010 Add parser test for duplicate extended references
Bug: T240459
Change-Id: Ifc7a695e89a49ccc6c66d49efe41b2321b0915f0
2019-12-11 15:58:34 +01:00
Thiemo Kreuz f92792f64a Fix bad localization of extended references numbers when reused
This adds a test for numbers like "1.2.0" that appear when an extended
reference (e.g. "1.2.") is reused multiple times.

The first separator is from the extended reference. We decided to never
localize it. However, the second seperator is from reusing a reference.
This was always localized. We believe this is a bug, but haven't fixed
it yet.

The test is documenting the status quo "1.2,0" with a comma. This kind
of makes sense, one could argue, because the "1.2" appears like this up
in the text, but the ",0" is a different indicator for a reuse, which
*never* occurs in the text.

Change-Id: Ie3d26bcadd8929b906bfbcac4806af2150d61f2a
2019-12-09 17:25:14 +01:00
Adam Wight f51060eaf4 Fix footnote mark after extends numbering glitch
The visible numbering needed to be rolled back after an extends.

Bug: T237241
Change-Id: I95404515110df1fa7e3279ea499577df0ed45ddf
2019-12-09 12:06:59 +01:00
jenkins-bot 3b41cfa472 Merge "Fail early on nested extends="…", if possible" 2019-12-09 10:12:54 +00:00
jenkins-bot 399a9c63bf Merge "Numbering bug: Parser test which should fail" 2019-12-09 10:06:06 +00:00
Thiemo Kreuz c5fe49ff11 Fail early on nested extends="…", if possible
This partly reverts Ied2e3f5. I haven't properly tested this before.
Rendering a bad extends (that extends a <ref> that's already extended)
not indented messes the order up and rips other extended <ref>s out of
context.

For now it might be better to stick to the previous, "magic" behavior:
Such an extends behaves like it is extending the *parent*, and is
ordered and indented as such. This is still not correct, but I feel
this is much better than rendering such a bad extends on the top level.

This patch also makes the code fail much earlier for a nested extends,
if this decision can be made already. In this case the error message is
rendered in the middle of the text (as other errors also are), not in
the <references> section.

Change-Id: I33c6a763cd6c11df09d10dfab73f955ed15e9d36
2019-12-09 10:54:52 +01:00
Adam Wight a91cf43154 Remove outdated TODOs
These edge cases are handled correctly already, I just forgot to
remove the TODOs when updating test content.

Note that there's only one TODO left, and it's to forbid a feature which
actually works!

Change-Id: I0d3a1f55f0ce943b0d034dda40e3779fbf241fe4
2019-12-09 10:25:19 +01:00
Adam Wight d8433101a7 Numbering bug: Parser test which should fail
Includes the TODO for what correct output looks like.

Bug: T237241
Change-Id: I0e60724f2c418b19e5affc24dca7f446c2b38bb3
2019-12-09 09:53:53 +01:00
Thiemo Kreuz 31bda4777b Don't indent refs with forbidden extends="…"
Change-Id: Ied2e3f56ce66d2a8ccf60df2bdbf99acad461595
2019-12-04 15:17:03 +00:00
Adam Wight 81261493c2 Show error when extending a subreference
Change-Id: Iaa47e302e5e49dfc190fde37567a3e7a2e743d67
2019-12-04 13:49:31 +01:00
Thiemo Kreuz 54333c9bd6 Stop formatting "1.2" as "1,2" in languages like German
The new extends="…" feature is using numbers like "1.2". These should be
localized in languages like Hebrew that uses other symbols for the digits.
But the "." should not change.

The existing feature when a <ref> is reused multiple times does have the
same "issue". But it seems this is intentional, because it is covered by
a test. Note this is not visible in German, because German uses custom
labels "a", "b", and so on.

This patch also improves the so called "smoke" tests and makes one cover
numbers up to "1,10" for a <ref> that is reused that often.

Bug: T239725
Change-Id: Iffcb56e1c7be09cefed9dabb1d6391eb6ad995ce
2019-12-04 09:43:04 +01:00
Adam Wight 008526b3aa Can use extends before its parent
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
2019-12-02 17:14:11 +01:00
Adam Wight 3f276388bf Split ref.number field
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
2019-12-02 10:17:24 +01:00
Adam Wight 00f3be7c7f Reset extendsCount after each group
This was a bug which would affect book references, if the same group
and parent ref name combination occur twice in an article.

Change-Id: I608f58aac0cec31c8650835fc80195a87bc851d3
2019-12-02 10:17:18 +01:00
Thiemo Kreuz 13598ba11e Render nested references
Forked from Icd933fc983.

Bugs and unimplemented features are documented as TODOs in the parser test
fixtures.

Bug: T237241
Change-Id: I9427e025ea0bcf2fa24fd539a775429cc64767cc
2019-11-29 13:40:34 +01:00
Adam Wight 22a0350d84 [Refactor] Pass validation error with StatusValue
This has clearer semantics than checking for a `false` attribute.

Change-Id: I68f777eda40f8f157deafacaed02d4bd10cbf25c
2019-11-27 18:05:19 +01:00
Thiemo Kreuz b10dd4ec27 Block de-facto empty <ref> as if it's empty
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
2019-11-20 15:07:54 +00:00
Adam Wight 9d706047f3 Rename refines -> extends
Bug: T171581
Change-Id: I42b2d8859f2958357024cbba089715c10712f370
2019-11-12 10:19:17 +00:00
Thiemo Kreuz fe385ecc37 Block all combinations of refines="…" and follows="…"
Note it doesn't make a difference if this is behind the feature flag or
not. It should always be forbidden, and in fact is: Either the follows
attribute is unknown, or the combination is forbidden.

Bug: T236256
Change-Id: Iebbb2d1d5bab183ab0590b8a7a7f6e79d319b72c
2019-11-11 12:56:58 +01:00
Thiemo Kreuz d919615e28 Merge bookReferencingUnimplemented.txt into bookReferencing.txt
What we find critical is:
* That all tests relevant for book referencing are in a separate file.
* That unimplemented stuff is marked with TODOs.

Not having to move tests to another file allows for nice diffs.

I tried to order the tests as good as I could. E.g. have all tests with
a group="…" next to each other, followed by all with a follow="…".

Change-Id: Idc1d9e7843b341235ab3d8ebe398e01946eb1845
2019-11-11 12:50:51 +01:00
Adam Wight 0ebf86fdf3 Split out BookReferencing parser tests
Encapsulate the feature tests in dedicated files.  These are picked
up by the test runner for matching glob `tests/parser/*.txt`, as can
be shown by,

  phpunit.php --testsuite parsertests --filter=bookRef

Also adds TODO comments to some tests, documenting how the current output
will not match the fully implemented code's results.

Bug: T236256
Change-Id: Ie3e769c84856256180754aeff417da893a84b479
2019-11-08 10:02:38 +01:00