Commit graph

39 commits

Author SHA1 Message Date
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 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 301b1fbcaa Move a follow edge case to validation
Change-Id: I06cf5291c258322e16449d61879bf7a18129b174
2019-11-26 15:28:02 +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
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 ea6cea93ed Move bad dir="…" error reporting down to the renderer
… and make the error message for bad dir="…" shorter and more to the
point.

Now I understand why the error reporting was not done when $text was
empty: the error was actually appended to $text, which messes with
everything else that also works with the $text variable! This even
includes the API. This error message was exposed via the API. That was
certainly a bug.

With this patch, all error checking for the dir="…" attribute is now
done way down, when rendering the <references> section.

Note this also fixes a bug where the dir="…" was *not* rendered when
previewing a section.

Change-Id: I4ab0cb510973ed879c606bfaa394aacc91129854
2019-11-22 10:07:28 +01:00
Thiemo Kreuz 65c8967c32 Fix internal presentation of the dir="…" attribute
This fixes a whole bunch of inconsistencies:

* The dir attribute is now trimmed, as most others already are. This is
an actual user-facing change.

* The internal representation is now false in case the value was invalid,
not an empty string any more.

* Null means the attribute was not present. This is now always used,
even in the return values that are meant to represent an error state. No
existing behavior changes.

* The internal representation does not contain an HTML snippet any more,
but the raw value "ltr" or "rtl", or null. Note this might influence the
API, because the API actually exposes the internal representation.
However, we are pretty sure the API is not used anywhere. Even if,
exposing HTML code was most certainly an unwanted and unexpected effect
of the patch that introduced the dir attribute. This does make this a
bugfix, I would argue.

Change-Id: Ic385d9ab36fa0545c374d3d63063028ae4e449d4
2019-11-21 12:52:47 +01:00
Thiemo Kreuz ab3063fee5 Move all code to PSR-4 compatible namespaces
This patch does intentionally not touch any file name. Some of the
file names are a little weird now, e.g. \Cite\Cite. These can more
easily be renamed in later patches.

I used https://codesearch.wmflabs.org/search/?q=new%20Cite%5C( and it
looks like this code is not used anywhere else.

Change-Id: I5f93a224e9cacf45b7a0d68c216a78723364dd96
2019-11-20 17:00:13 +01:00
jenkins-bot 32e1f8e7c3 Merge "Don't pass a Title object around that's not needed" 2019-11-19 16:09:13 +00:00
jenkins-bot 7018e82352 Merge "Extract all error reporting to a CiteErrorReporter" 2019-11-19 15:53:29 +00:00
Thiemo Kreuz 9d2d61ff09 Don't pass a Title object around that's not needed
Change-Id: Iea9c366c4b45ba4cd9171c8b4fffc307c852b6e2
2019-11-19 16:48:36 +01:00
Thiemo Kreuz 342e231a22 Extract all error reporting to a CiteErrorReporter
Change-Id: Icf61c9a27fd03266c98caf443bb9f00a421e31f6
2019-11-19 14:53:31 +01:00
Thiemo Kreuz 7157c7f494 Add @license to all files
Note this codebase appears to be dual-licensed. Some files mention MIT,
but extension.json and some other files mention GPL.

Since WMDE typically uses GPL, I will continue to mark the files we
created as such.

Change-Id: I126da10f7fb13a6d4c99e96e72d024b2e5ecee06
2019-11-19 11:31:08 +01:00
Thiemo Kreuz d50c169612 Minor test updates for more complete test coverage
The main motivation here is to cover the fallback code that was moved
in I20c814d. At some point we might touch this code again.

Bug: T238194
Change-Id: I0ab8a34b09790f42b10376eb3730c3b3c4ef53d2
2019-11-14 14:42:22 +00:00
jenkins-bot 668ad80c58 Merge "Pass ParserOutput as parameter to Cite::checkRefsNoReferences" 2019-11-13 08:48:30 +00:00
Thiemo Kreuz 7920ec3150 Pass ParserOutput as parameter to Cite::checkRefsNoReferences
Change-Id: Ibc4455dfde9f60bb27eac0d71064796878994bc5
2019-11-12 16:33:52 +01:00
Thiemo Kreuz e68b96f75c Add more basic tests for API and RL modules
Change-Id: I5e54fae041ec8431c170be468c12f0622e355b9b
2019-11-12 16:32:15 +01:00
jenkins-bot 0782f24d31 Merge "Make most existing Cite tests pure unit tests" 2019-11-12 14:44:24 +00:00
Thiemo Kreuz f94b400474 Make most existing Cite tests pure unit tests
1. Most existing CiteTests can be unit tests. They run so much faster
this way.

2. I modified some test cases to cover all trim() in the code.

3. The strict type hint in CiteHooks is removed because the parameter
is not used. Having a hard type hint for what is effectively dead code
makes the code more brittle for changes done outside of this codebase.

Change-Id: I1bff1d6e02d9ef17d5e6b66aeec3ee42bba99cf4
2019-11-12 14:56:40 +01:00
Thiemo Kreuz d8fbbd0037 Remove dependency on PPFrame from Cite class
This fixes a series of issues:
* There is nothing about a "frame" in the Cite class any more.
* There is no addModules() call in the Cite class any more.

Change-Id: I20c814d46c26825c5c07eab0a5586de3a531eee7
2019-11-12 13:06:39 +01:00
jenkins-bot f36be06996 Merge "Add basic unit tests for all 3 hook classes" 2019-11-12 11:38:13 +00:00
jenkins-bot 59dba7e184 Merge "Remove lazy registration of Parser related hooks" 2019-11-12 11:20:57 +00:00
Thiemo Kreuz 042a4ecf7a Add basic unit tests for all 3 hook classes
Change-Id: Ib444717465f8dda96c89afd8b2d60336e8bcdeec
2019-11-12 11:11:45 +00:00
Thiemo Kreuz 7ce10d7539 Remove lazy registration of Parser related hooks
To be honest I don't get why this lazy registration was done in the
first place. None of the 4 other hooks should ever be called before
the ParserFirstCallInit hook got called.

Also, under which circumstances can the ParserFirstCallInit hook be
called more than once?

Both scenarios would be wrong, as far as I'm concerned. Either I'm
missing something, or this code can indeed be simplified. Maybe it was
something to make it more compatible with older MediaWiki versions?

The only reason I can think of is: in all situations that do not
involve a parser, having the 4 extra hooks registered is pointless.
Does this waste space and/or runtime in the $wgHooks registry?

Change-Id: I5ef1495f4ce7bce940fa5f8e700af3d2c4851a01
2019-11-12 11:47:55 +01:00
Adam Wight 9d706047f3 Rename refines -> extends
Bug: T171581
Change-Id: I42b2d8859f2958357024cbba089715c10712f370
2019-11-12 10:19:17 +00:00
Thiemo Kreuz 818e869b0b More narrow method signatures involving Parser
Change-Id: I2da717b9a8d104644c59a62b49090605c95323d6
2019-11-12 10:24:58 +01:00
jenkins-bot 657b81abd2 Merge "Add basic test coverage for all CiteHooks code" 2019-11-11 19:24:06 +00:00
jenkins-bot 15a84769b8 Merge "Test cleanup: drop equalTo" 2019-11-11 12:43:04 +00:00
jenkins-bot 62ca80536e Merge "Block all combinations of refines="…" and follows="…"" 2019-11-11 12:37:34 +00:00
Adam Wight bde9e175a8 Test cleanup: drop equalTo
Can use a shortcut where we pass the expected value directly.  Verified
that we're still asserting equality.

Change-Id: I63512488c50e599df23d5dae2a5064218e311e90
2019-11-11 12:57:09 +01: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
Adam Wight b7a7457ffd Add page property when parsing book reference
Any time the book referencing attribute is used in a page,
permanently tag that page with the `ref-extends` property, so
that it can be watched and cleaned up if necessary.

Bug: T237531
Change-Id: Ice5d9d8f7a305702cdc7c2a55d4147c4f79b5881
2019-11-11 11:06:31 +01:00
Thiemo Kreuz ae01d35bf2 Add basic test coverage for all CiteHooks code
This also updates an existing test to cover all trim() in the code.

Change-Id: I0f0b4f8154004f941f4eaa5a9b2c3be0598fb137
2019-11-08 15:59:01 +01:00
Adam Wight 5ac57def59 refArg parses and returns the refines attribute
Incremental patch which extracts the refines attribute from the tag.
Doing this now to allow the calling function to have responsibility
for doing something with the attribute value.

Bug: T237531
Change-Id: I59bb409bedd8e6ed06268e705e02e8ffb45b1f0e
2019-11-08 12:30:12 +01:00
Thiemo Kreuz 7965659b82 Add dedicated unit test for Cite::refArg()
Again, this is intentionally testing a private method.

Change-Id: I559b88e38f7a7a4128ba0b16ff3de42f2fab2055
2019-11-07 13:14:05 +01:00
Thiemo Kreuz 64c94662f0 Add the first small PHPUnit test for Cite::normalizeKey()
Note this is intentionally testing a private method. As of now, the
code is so heavily entangled, it's not yet possible to test individual
aspects without calling private methods. The plan is to slowly increase
the overall test coverage, and the start restructuring the code as
necessary.

Change-Id: Ib3b01bddaffd0469fb66979c67c8114a5807df6d
2019-11-07 09:23:13 +00:00