* 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
There is currently no strict CodeSniffer rule for this. I think we
need to have one sooner or later. Anyway, what I find important is to
have a consistent code style in one codebase.
I refused to do this change previously because I don't like to mess
with Git blame if it's not really necessary. However, at this point all
code was moved around anyway.
I ended removing a comment that appears misplaced now, and doesn't help
maiing the code more readable. I like not having a dot at the end if
it's not really a sentence.
Change-Id: Id1d4f43277c69080c512c1a5ceff4c948bfa05be
In the end I don't care much if we agree on having this newline, or
not. What I care about more is that this codebase is consistent.
Personally I prefer having the newline. It creates a visible separation
between what "goes in" and what "goes out" (@throws and @return).
Change-Id: Ibc60af621132e415a5579397c01688fa21eb0be5
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
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
The rollbackRefs function no longer needs to "know" details about
how to turn a refCallStack item into a redo item. This is better a
responsibility of the subroutine, where the items are unpacked.
Change-Id: I1e2ff77cb5e66d70e451ee09e641ff752c770ab4
The logic was changed in 51ff3cc819.
Only `responsive="0"` is supposed to disable responsive references,
any other value should enable it.
Bug: T241303
Change-Id: I8c99bf93c739d6dba348785b1b6452cfce2c57c9
One of the most significant changes is when I noticed that the $group
can never be null. We set it to DEFAULT_GROUP before. That's an empty
string.
I'm not very happy with the two @phan-suppress-next-line. Is there a
better way to fix these lines?
Change-Id: I33c1681e2f3857cb6701da71f4ed8893caff4d1e
These should be impossible conditions, we don't want to continue with
processing.
I hate this patch, it's a temporary workaround until someone rewrites
or replaces the rollback logic, for example with a two-pass parse.
Change-Id: I6a1327e397d4272fa412c3f290c2107d867d2854
I hope this patch is not to horrifying and can be reviewed. It's
possible to split this into a sequence of smaller patches. Please
tell me.
Change-Id: I4797fcd5612fcffb0df6c29ff575dd05f278bd4d
The main benefit is this nifty call: `$this->rollbackRef( ...$call )`
To make this possible, the minimal change I needed to do was to move
the two $argv and $text arguments to the end.
I also tried to order all other arguments as good as I could: Required
first, optional later. Group and name together. Name and extends
together.
All this is private implementation and should not affect anything.
Change-Id: I7af7636c465769aa53122eb40d964eabdd1289ba
I feel this is a little better than before. It looks like we never need
to *replace* a text that existed before.
This depends on I4a156aa which fixes one of the last remaining trimming
issues. Outside of <references>, a <ref> </ref> with no other content
but some whitespace was already forbidden. But not inside of <references>.
This is relevant for appendText(). It should not be called with null, but
was because of the inconsistent behavior.
Change-Id: I38c9929f2fa6e69482e45919e2f8dbf823cb1c8b
Note that this patch changes behavior, an invalid "dir" will result in
a cite reference at the point where the <ref> is declared rather than
in the references section. This is consistent with other errors.
Bug: T15673
Change-Id: Id10db40aa0b391f2f1d9274aa09d22a7278d65e3
This makes one of the last remaining edge-cases about non-empty, but
non-visible content (a <ref> that only contains whitespace) behave
identical to all other places. We already reported it as being empty
everywhere else, except inside of <references>.
Note that the test cases look like they are reporting the same errors
twice. But this is not the case:
The first set of errors is about <ref name="…"> inside of <references>
not having visible content. This should always be reported, even if the
<ref> got content from somewhere else on the page.
The second set of errors is when a <ref name="…"> *never* got any
content.
This patch will slightly increase the numbers of errors reported.
Change-Id: I4a156aa9e466f735d92fe0ba5cc0678ec8bbdd50
* Use the Html class to safely create HTML code.
* $this->referenceStack can not be null any more.
* $this->inReferencesGroup is not needed during output, only when
parsing tags.
* Replace ReferencesStack::getGroupRefs() as well as deleteGroup()
with a combined popGroup() that does both things.
* Extract the code responsible for the "responsive" behavior to a
separate function.
* Some TestingAccessWrapper are not needed.
Change-Id: Ie1cf2533d7417ae2f6647664ff1145e37b814a39
In these cases, an expression is either true-ish or null, so we can use
the implicit boolean cast to test.
Change-Id: Ibe94829f9774bf2a1907635a8bd28369908b4d1e
Finishes breaking the circular reference between Cite and Parser.
This patch also demonstrates how evil it is to allow the error reporter
to be called from anywhere, and have side-effects. At least it's explicit
now.
Also fixes a bug where the inner error message would not be in the
interface language.
Bug: T240431
Change-Id: Ic3325cafb503e78295d72231ac6da5c121402def
This begins our journey of breaking the circular reference between
Cite and Parser. In later patches the child objects will also take
Parser as a parameter.
Bug: T240431
Change-Id: Ic672bb4bae19ac5f1e1f5817de171d76b3bd8786
Only create a Cite object if we need one. Never clearState, just
destroy and recreate later.
This makes it less likely that we leak state between parsers, and
saves memory and processing on pages without references.
It's also preparation to decouple Cite logic from state.
Change-Id: I3db517591f4131c23151c76c223af7419cc00ae9
* All classes are in a Cite\ namespace now. No need to repeat the word
"Cite" all over the place.
* The "key formatter" is more an ID or anchor formatter. The strings it
returns are all used in id="…" attributes, as well as in href="#…" links
to jump to these IDs.
* This patch also removes quite a bunch of callbacks from tests that
don't need to be callbacks.
* I'm also replacing all json_encode().
* To make the test code more readable, I shorten a bunch of variable
names to e.g. $msg. The fact they are mocks is still relevant, and still
visible because these variable names are only used in very short scopes.
Change-Id: I2bd7c731efd815bcdc5d33bccb0c8e280d55bd06
Fixes a bug introduced in Icf61c9a27fd, which would cause a parser
cache split any time the Cite extension was initialized. The
`setLanguage` interface is regrettable, but I'm hoping it will only
be around temporarily.
Converts an integration test into a unit test and completes coverage.
Bug: T239988
Change-Id: I4b1f8909700845c9fa0cbc1a3de50ee7d42f69a5
Because that is what it does. Note our method is different from the one
in the Language class. We only accept strings.
Change-Id: I39107e837cc29f2d7c8867c1e602aa643f9e1a57
This is what happens:
* The issue happens only on pages with two <ref> tags than share the
same name and group, but have conflicting text.
* This triggers a code path that renders an error message and calls
Message::plain() as well as Parser::addTrackingCategory(), which calls
Message::text().
* The Message class is asking for a new, fresh parser. This means the
parser is cloned and it's state cleared, while keeping stuff like
parser hooks.
* Cloning the parser triggers the ParserCloned hook.
* The hook handler clones the Cite instance stored in Parser::$extCite.
* PHP doesn't do deep cloning. Object properties are not cloned.
* Since I091a0b7 the internal state of the Cite class is extracted to
another class.
* This means the state is not cloned any more since I091a0b7.
* Now two Cite instances share the same state.
* At the end of the hook handler, the state is cleared, which also
clears the state of the original instance.
We will most probably solve this on master by getting rid of cloning
Cite. We propose this additional hotfix for the branch.
Bug: T240248
Change-Id: Ic5a438e04d003a637ae08aae936d9977cc90d5d3
This class renders a <references> tag and everything inside. The
previous name sounds like it is responsible for rendering the contents
of a <ref>…</ref> tag. I mean, the class contains a method that does
exactly this. But this method is private.
Change-Id: I1cd06c9a11e0a74104f2874a34efa3e0843a0f70
