Commit graph

2832 commits

Author SHA1 Message Date
jenkins-bot 7ed54a3f3d Merge "Remove redundant attribute trimming" 2019-11-27 12:26:00 +00:00
Thiemo Kreuz 99d23ac841 Remove redundant attribute trimming
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
2019-11-27 12:12:51 +00:00
Thiemo Kreuz 04f784bc02 Remove non-existing property from ReferenceStack
Change-Id: Id789897dd92c7692e36f54b828c097820ab46b43
2019-11-27 12:57:55 +01:00
jenkins-bot 36952a55a1 Merge "Add test to cover Cite::listToText()" 2019-11-27 11:46:44 +00:00
Thiemo Kreuz 2ffcae0425 Add seperate unit test cases for Cite::testValidateRef()
Change-Id: I6008b834d18c2008304b51dd41f0387c28e53d94
2019-11-27 12:09:31 +01:00
Thiemo Kreuz f5b9360467 Add test to cover Cite::listToText()
As far as I can see this must (for now) be an integration test because
it is calling wfMessage().

Change-Id: Ic581c38128364990ccf81539996d1dda53bdcda5
2019-11-27 11:59:53 +01:00
jenkins-bot 40942620b2 Merge "Rename ambiguous tests to …UnitTest" 2019-11-27 10:51:19 +00:00
Thiemo Kreuz 9f1521a773 Rename ambiguous tests to …UnitTest
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
2019-11-27 11:15:39 +01:00
Thiemo Kreuz a6a16f0703 Update and increase ReferenceStack test coverage
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
2019-11-27 11:08:00 +01:00
Translation updater bot ff47bc8092 Localisation updates from https://translatewiki.net.
Change-Id: I3c2f63822114c5dd77b730c19abf7dbca218ba1e
2019-11-27 09:36:29 +01:00
Adam Wight 97746db93c More specific function signature
Change-Id: Id1d21b9abdb11bbf441650ea0a1cccc8b258d598
2019-11-26 18:27:16 +01:00
Adam Wight ec091fe906 Reorder keys
This doesn't make any functional difference, but helps minimize later
patch Ida9612d14

Change-Id: Ice89bad02e077437d0df6fa9f51f90b4cab4837c
2019-11-26 17:06:01 +01:00
Adam Wight 38d7c09495 Tweak comment
This makes it show up nicely in my IDE.

Change-Id: Ic203a9dcb83c96c3324996e183b4dfc239f65eca
2019-11-26 17:03:40 +01:00
Adam Wight 890e86a7fb Fix tests: cannot have name and follow
This was impossible and is prevented by validation, so do not test.

Change-Id: I836b38c700f41f692e5c6a893be0076febfc9c4d
2019-11-26 16:57:56 +01:00
Adam Wight 55099a7b0c Remove impossible condition
Numeric `$name` is caught during validation.

Change-Id: Id1c3e6717af38b0b1393c135732e084d261b53f6
2019-11-26 16:43:21 +01:00
Adam Wight 7cdcc2b075 Alphabetize returned array of attributes
That was annoying me.  Since we're passing a bare list, alphabetical
order helps make the code and tests readable.

Change-Id: I6384094e429e0e2a6fa810fdc28ae0643a0ccf7c
2019-11-26 15:28:02 +01:00
Adam Wight 6a9e51fc30 Tag every usage of "extends" even when invalid
We weren't recording usages which failed certain `refArg` validations.

Bug: T237531
Change-Id: Ibcb875c5d0ed6c2279e0e34ab415ac63d7ebe584
2019-11-26 15:28:02 +01:00
Adam Wight 2229f22899 [Refactor] Nest conditionals with common term
Previewing is an important exception, so I wanted to consolidate and
emphasize its edge cases.

Change-Id: Iae343ed8c225407e8184ff09e426d531c9f6ab00
2019-11-26 15:28:02 +01:00
Adam Wight 301b1fbcaa Move a follow edge case to validation
Change-Id: I06cf5291c258322e16449d61879bf7a18129b174
2019-11-26 15:28:02 +01:00
Adam Wight 44a2599ab4 [Refactor] Handle extends attribute in validation
Moves logic so that `refArg` handles parsing and `validateRef` more
of the validation.

Change-Id: I2c0a789d5f2c20b1968c4809b5780d9fe738fd9a
2019-11-26 15:28:02 +01:00
jenkins-bot 71f6b91964 Merge "Add some comments" 2019-11-26 14:22:50 +00:00
jenkins-bot b78cdd888d Merge "Signature: these are all nullable" 2019-11-26 14:22:50 +00:00
jenkins-bot 68ade8af25 Merge "Drop unused param" 2019-11-26 14:22:49 +00:00
jenkins-bot 626e6ab00c Merge "Comments in TODOs" 2019-11-26 14:22:48 +00:00
jenkins-bot dfb0ccdac6 Merge "Test for numeric extends attribute" 2019-11-26 14:22:39 +00:00
jenkins-bot ae28a12e31 Merge "Test for numeric attributes earlier" 2019-11-26 14:20:57 +00:00
jenkins-bot 830755d5fb Merge "[Refactor] Consolidate validation" 2019-11-26 14:19:44 +00:00
Translation updater bot 7b92fe355a Localisation updates from https://translatewiki.net.
Change-Id: I71d3b80d66e9ed3a3f38d24b00064fe4aa472818
2019-11-26 11:42:18 +01:00
Adam Wight d1e0e6309b Add some comments
Change-Id: I012ad6cc52dc65f1b329febdc8e6441ac03c6463
2019-11-26 11:07:48 +01:00
Adam Wight 9fa22b15be Signature: these are all nullable
Change-Id: I79432de9007f9016604cb871b68577f1cf596be5
2019-11-26 11:07:39 +01:00
Adam Wight d65bb667b7 Drop unused param
The "dir" param is not validated, so don't pass through.

Change-Id: I78eaa3bf7067c6283a2d8e452b68f93ffab43875
2019-11-26 11:07:39 +01:00
Adam Wight 33014bb8f5 Comments in TODOs
Stuff we have to fix in future work.  Follows up on I2d9904b7631d0d6

Change-Id: I7bca73d3e9d3d2a224604efbe81d48948d2a3d76
2019-11-26 11:07:12 +01:00
Adam Wight 22b0bdf526 Test for numeric extends attribute
This is invalid, because it would allow access to internal, autoindexed,
anonymous refs.  These would break when refs are reordered.

Bug: T151305
Change-Id: Ib4bb8270d810b64e4c160f377ce52ce2fc70bab4
2019-11-26 11:07:11 +01:00
Adam Wight feaa724efa Test for numeric attributes earlier
This introduces a slight behavior change, but for the better:
* When pointing to the name "0", the non-numeric error will be displayed,
  which is correct whereas "no key" is not.

Change-Id: I33467b27cd447812fe67204831909c4d9869db08
2019-11-26 11:06:15 +01:00
Adam Wight 3fbeed0304 [Refactor] Consolidate validation
Validation logic can be split from arg parsing, default values and
other side-effects.

No behavior was changed.

Change-Id: I2d9904b7631d0d6be13e0aaed0106f186d388c4f
2019-11-26 09:25:06 +01:00
Translation updater bot 1d2885cd3e Localisation updates from https://translatewiki.net.
Change-Id: I9c658c7036b90e4d67b434ff98dedc396617dbee
2019-11-26 09:16:52 +01:00
Adam Wight 9e2468882d Cache parser previewing state
We need to access this in several places, store as instance state for
convenience.

Change-Id: I4ea8f279a34cd8f819d9c07c75e3e8e160786f9b
2019-11-25 14:06:55 +01:00
Adam Wight 8453e3ecd7 Extract stack and state to a new class
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
2019-11-25 14:06:32 +01:00
Adam Wight 10e4a4353d Finish renaming "key" to "name"
Change-Id: Iffd49268cfefefdce9c970f60b5d231e79cbc267
2019-11-25 13:25:40 +01:00
Adam Wight a0f019e1ac Clean up variable names in two more functions
Change-Id: I7e0eb97123a53463133226ae2067d2396e8ceda3
2019-11-25 12:39:57 +01:00
Adam Wight 3ec5a7c3e7 Clean up comments
Clarify where different keys come from and what they're used for.

Change-Id: I534de4952c5b0e053dcba95f31c837547b4a68a6
2019-11-25 12:37:07 +01:00
Adam Wight b9c51b81a1 Rename variable to $name
This is still directly fromt the "name" attribute.

Change-Id: I2c3d0faf591be7e5032c0b26cf5eb7542390bd64
2019-11-25 12:35:42 +01:00
Adam Wight 17c5d6e981 Use "name" to be consistent with attributes
Call the variable the same thing as the attribute it comes from.

Change-Id: I012e29018bdabadc7ec87ed13fc396a7a653933e
2019-11-25 12:31:32 +01:00
Adam Wight fb430f257e Fix variadic parameters in error reporter
Give these functions the same signature as `->msg()`

Change-Id: Ib90df52d6752512d7d9dddf51777c9c23c847e06
2019-11-25 11:02:31 +00:00
Translation updater bot 683b4e3e6b Localisation updates from https://translatewiki.net.
Change-Id: I238c18b40b4d12376b6055efa0bc5b6eca278a59
2019-11-25 09:09:29 +01:00
Thiemo Kreuz (WMDE) 5401fcd190 Revert a part of "Add missing test cases for follow="…""
This partly reverts commit 8e42a6ecdf.

The variabe $k as created by the foreach() loop is not necessarily
numeric, because the $this->mRef structure contains data both for
named and unnamed <ref>s. The array key is a (non-numeric) string for
named, and an integer for unnamed <ref>s.

array_splice() requires a position, not an array key.

Note that both implementations are wrong. The foreach() might return a
string $k, which makes array_splice() complain and do unwanted things.
The for() loop assumes there are count() array elements with integer
keys, which might not be true. Luckily this was not a problem, because
the isset() check would stop the (to long) loop eary enough.

A better rewrite as well as a test case for this will be added with
I3bdf26f.

Change-Id: I5568d3084197f1861f9dc8983d8b606a961e201f
2019-11-23 22:11:17 +01:00
Subramanya Sastry 2cfb76f8b6 Sync up with Parsoid citeParserTests.txt
This now aligns with Parsoid commit 7dfc2e931a6afeb62d2a0d791cda88fd8d39c070

Change-Id: I7edd1f293530653ae1bbfe47028e585f2b46927b
2019-11-22 18:44:22 +00:00
Thiemo Kreuz c76a5e84f9 Fix misleading method names in CiteErrorReporter
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
2019-11-22 15:08:39 +01:00
Thiemo Kreuz 177c9cc1eb Fix inconsistencies and deep nesting for follow="…"
* 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
2019-11-22 15:01:09 +01:00
Thiemo Kreuz a823fa23d9 Merge two more code paths in Cite::referencesFormatEntry()
There was another, duplicated special case for previews. It was using
the same message as a <ref> with multiple uses. Now it's only one code
path.

The goal here is to reduce the number of code paths to make it much
easier to implement proper rendering for the extends="…" use cases.

Bug: T237241
Change-Id: I863ac3b5234d3a6f7f2371a2a85385c3aea276e5
2019-11-22 15:01:09 +01:00