In 47506af, serializing references content to be added to data-mw was
delayed until we were inserting the reference into the dom, to give us a
chance to mark up errors about not finding named ref content. After
which, the content is cleaned out of references to make room for list
items that belong there.
In 6bd0594, we noted that we can't store linkback from embeded content
since the content is released after serializing and won't be available
when it comes time to mark it up with errors.
Similarly, the linkbacks added to other groups from the references
content won't be available after inserting that reference and so we
shouldn't be holding on to them either. This means that we won't be
marking them up for the named error above but with the benefit that we
won't crash when trying to access these linkbacks from another group.
A test case is added which crashes trying to access a linkback from a
group that has already been inserted.
Note that the extension to the legacy parser considers referencing
another group from group content to be
"cite_error_references_group_mismatch". If we choose to also do that,
we may be able to reverse this change.
Change-Id: Idf0e49fa07dc3614068793c72a30ce3de1e2392c
* Note: the <ref name="1"> numeric is flagged as an error, but
<ref follow="1"> is not flagged as well as it would be duplicitous.
We are proposing that Numeric digits as a name be changed to
a warning as Parsoid Cite handles them properly without error.
Background: The core Cite extension started off not allowing numeric
names because the data structure they used made it inconvenient,
but Parsoid Cite has a separate index for named refs.
The core Cite devs thought there was potential for a conflict in
the ids if numeric names were allowed. The ids that core uses
follow the pattern:
for no name defined: cite_ref-1 cite_ref-2
for name="refname": cite_ref-refname_3-0 cite_ref-refname_3-1
so for name="1" at worst you might see ids like:
cite_ref-1 or cite_ref-1_2-0
so that does not produce conflicting IDs and isn't a concern,
and that is the pattern that Parsoid Cite uses.
* Error case of numeric in name and follow and two tests that
validate errors.
Bug: T51538
Change-Id: I95d725d0f77abadc1ddb2dd6939762b7d322e4f2
* Error case of dir= not ltr or rtl is caught and a cite tests
validates the new check.
Bug: T51538
Change-Id: I8c7e088416f0a000e638771a3fe5e8e0c58bcc23
Avoids crashers from trying to serialize the named content twice if
there's a valid follow with some other error, as expected in the FIXME.
This introduces a backwards incompatibility for invalid follows, which
will result in the contents of the ref being dropped.
A test is added to assert that selser will save us for the most part.
Change-Id: I1f572f996a7c2b3b852752f5348ebb60d8e21c47
* This patch introduces a preprocessing step on the edited DOM.
* Existing preprocessing code has been extracted into the
preprocessDOM method.
Any registered extensions preprocessors are invoked on the DOM.
So, this assumes that the htmlPreprocess extension listener is only
applicable to the edited DOM. If we want to expose the concept of
selective serialization through the API, we may want to add an
additional interface method / listener to the DOMProcessor class.
As of this patch, this is somewhat theoretical since there are no
such extension handlers registered on either DOM. Future patches
can clarify this better as specific needs arise.
* The handler also calls the serializer's custom preprocessing steps.
This step is applicable to both the original as well as edited DOM
(since DOM Diff is impacted by the results). If a need arises,
in the future, we may introduce a new extension DOM processor method
that applies to both original and edited DOMs.
* Right now, only selser strips section tags and non-selser wts
doesn't need to. So, preprocessDOM there is empty. Additional
selser-only DOM preprocessing will show up in later patches.
* Moved a stub HTML->WT preprocessor in Cite extension to RefProcessor.
Bug: T254501
Change-Id: I0c12afb2ea82617406d72ad872ac4f33678fa5f2
The description in T179082 suggests that by using one document for the
entire parse, we'd probably see some performance gains from not having
to import nodes when we get to the top level pipeline and we'd avoid the
validation errors from 19a9c3c.
However, the spec seems to suggest creating a new document when parsing
an HTML fragment,
https://html.spec.whatwg.org/#html-fragment-parsing-algorithm
And, indeed, domino implements it that way,
12a5f67136/lib/htmlelts.js (L84-L96)
So, the request in T217705 may be a little misguided.
What then is this patch good for? In T221790 the ask is that
sub-pipelines produce DocumentFragment which make for cleaner interfaces
and less confusion when migrating children.
The general outline here is that a document is created when the
environment is constructed that gives us the 1-1 correspondence.
Sub-pipelines do create their own documents for the purpose of tree
building, as in the fragment parsing algorithm, but are then immediately
imported to DocumentFragments to be used for the rest of the
post-processing passes.
Bug: T221790
Bug: T179082
Bug: T217705
Change-Id: Idf856d4e071d742ca38486c8ab402e39b3c8949f
* Remove the id's from follow refs because they were
duplicating the same key value erroneously and also
did not provide useful info. Fixed all tests accordingly.
* Added FIXME which refers to a new Phab ticket about
removing the code which adds style = display-none
that will be moved to CSS at some point.
Bug: T262986
Change-Id: Ib59f5eec951aa83a02357de865df8ab3dd8d2f67
These refs get a `style="display: none;"` since they're
not intended to be user visible.
Follow refs with errors conform to the proposed spec in T251842
Bug: T51538
Change-Id: Ie4ea28e7f9afde24614874bb4b8e07c5cabafa12
* Interim state commit with experimental code.
* Updates to citeParserTests.txt to check now valid follow
functionality and newly passing tests.
* Added to follow refs, <sup style="display: none;" about=...
to suppress display of hidden sups needed for VE to use
in editing follow refs.
* Added code to implemented follow functionality and catch
invalid usage.
Bug: T51538
Change-Id: Ic3ac8237fd2c490cfaf2fe799759742f72f10686
This is done to make the discussion in If3dcfd7 easier.
When we introduced this code we actually used it to format
entire numbers. We had to change this later to *not* localize
digits, but only separators. Language::formatNum is and always
was able to do this, so we just continued to use it.
This is discussed now.
It turns out there is only a single place left where we use
formatNum, and it does nothing but localizing the decimal
point. There is another way to do the same.
Bug: T237467
Change-Id: I89b17a9e11b3afc6c653ba7ccc6ff84c37863b66
* Bug fix for accessing undefined extsrc member variable in edge
case. See T260082 for deeper explanation of the WT that caused a
case where empty flag is not set and extsrc is also missing, but
since either case including extsrc being unset indicates no
content, this additional check is safe for now.
Bug: T259676
Change-Id: I20750c6977883668c83bdae78fbeb171f899e1ab
* The latter feels more readable and intuitive.
* The option defaults to true.
* To make for simpler code, I ensured that the option value is always
set before it is accessed.
* But, the typeof value still uses the "/sealed/" qualifier.
Alternatively, I could use "/packed/" if we want to adhere to the
config value name more closely.
* Tangentially related changes:
- made getWrapperTokens a private method since it is only used
internal to PipelineUtils.
- remove default value for $opts in encapsulateExpansionHTML since
the value is always passed in everywhere.
Change-Id: I86c4e5adf11e3151f51f2623e5ed85282a2e1298
This was done with a custom sniff in,
MediaWiki/Sniffs/Commenting/FunctionCommentSniff.php
`$singleType === 'null' && count( $explodedType ) === 2`
since there's some ambiguity with,
`what|type|null`
but also a case like the following is left out,
`string[]|null`
Change-Id: I1bd50a4486d7ef4974280b476fd03d3ee53232b3
Alternative to I6ea271a5d5c7b12a13bb12a682c39bcfd7b1f116
We can follow this up by passing the ExtensionTag to the
ExtensionTagHandler constructor.
Change-Id: I5b1b191bc85968ad617eb3ebcdd7721c55006af2
We broke this feature in December 2019 because it was never covered
by any tests. Full explanation in T245376.
All the features we care about are covered by tests. If all existing
tests succeed, that should be proof enough that this patch does not
introduce any new regression.
Bug: T245376
Change-Id: I1a447884bdc507ac762d212466496b4591c18090
* Bug fixes to accurately match core cite use of
cite_error_ref_no_key error and adjusted citeTests to match.
Bug: T51538
Change-Id: I3ae5300a5f86decebb7e67c5ea57c0c15677fbcc
* named refs which attempt to redefine the content are flagged with
an error, but not follow on named refs that leave content blank
or repeats the original content.
* Fixed cite tests affected by this change to include !! html/parsoid
sections.
Change-Id: I6832603c523a0465a6cc08f68c9ca79499331cd7
Keep the contents in the references and serialize at the end, which
allows us to mark up the errors there.
This is a follow up to 4438a72 which resolves the FIXMEs added in
8cb34b6.
Change-Id: Ia5b5cdbd0e9f3b5c558b8bbc5eb4b9955f4922c9
Also, FIXMEs for a follow up to 4438a72 that's exposed by this test.
Nested refs in references aren't getting marked up for the
"cite_error_ref_no_text" errors, where applicable.
Change-Id: Ie6e461571402a96e47d3df26585d9a40f1038891
* This lets us expand the range of available contexts in the future
without needing API changes.
* This patch only touches extension and extension API code. Parsoid
internal code can be changed independently.
Change-Id: I51d4c2120a31efb6dbb409926f8f8dad61f4dcc3
* Detects grouped and named refs that fail to define content.
* Uses group and name ref list tracking info to back patch
'mw:Error' and i18n error key string into the data-mw
section of all instances of named refs that all fail to
define content.
* The failures for test References: 7b is because selser is
arguable smarter than wt2wt. The newline before the references
list has been randomly deleted but selser manages to restore it
from source. wt2wt doesn't put the references tag on a line by
itself, even though it asks for block format, because it isn't
a new list - (these comments are from Arlo's review)
* Added test: "References: 7b. Multiple references tags some with
errors..." to ensure that refs with and without content errors
grouped and named do not cross references section boundaries.
Bug: T51538
Change-Id: I884fc337165506c5abbef18bcd5a5fca015786d2
Additional changes:
* Removed phan-taint-check-plugin from extra, now inherited from mediawiki-phan-config.
Change-Id: I280ee7f72faecad666cb088be9950f9a5250c9c9
Mediawiki prefers to use an object factory pattern when creating objects.
Use ObjectFactory consistently when creating objects specified using the
extension API. Thanks to the 'allowClassName' option to
ObjectFactory::getObjectFromSpec(), this is mostly consistent with
previous practice. Note that the string class name is short for:
[ 'class' => Foo::class ]
and so we've chosen to rename the 'class' property in the extension tag
configuration so that we have (in long form):
[ 'name' => 'Cite', 'handler' => [ 'class' => Cite::class ] ]
instead of nesting two keys named 'class' in a row. (And besides, the
content isn't really a 'class' any more, it's an "object factory
specification".)
SiteConfig::registerExtensionModule() can now take *either* an object
factory specification for an ExtensionModule object (including a bare
class-string) *or* the contents of the configuration array that
would be returned by ExtensionModule::getConfig(), in which case it
creates an anonymous ExtensionModule object for you. It's expected
that the latter will be preferred in extension.json, but we use the
former for our internal extension implementations at the moment.
Finally, call SiteConfig::registerExtensionModule() on the results
of ExtensionRegistery::getInstance()->getAttribute('ParsoidModules')
when running in integrated mode. This allows you to register your
extension with a clause such as the following in your extension.json:
(simple case, naming a class which implements ExtensionModule)
{
"name": "JsonExtension",
"manifest_version": 2,
...
"ParsoidModules": [ "Wikimedia\\Parsoid\\Ext\\JSON" ]
}
(complex case, putting the configuration array into extension.json)
{
"name": "Cite",
"manifest_version": 2,
...
"ParsoidModules": [
{
"name": "Cite",
"domProcessors": [
"Wikimedia\\Parsoid\\Ext\\Cite\\RefProcessor",
],
"tags": [
{
"name": "ref",
"handler": "Wikimedia\\Parsoid\\Ext\\Cite\\Ref",
"options": {
"wt2html": { "sealFragment": true }
},
},
{
"name": "references",
"handler": "Wikimedia\\Parsoid\\Ext\\Cite\\References",
"options": {
"html2wt": { "format": "block" }
},
}
],
"styles": [
"ext.cite.style",
"ext.cite.styles"
]
}
]
}
The syntax above, with `ParsoidModules` as a top-level attribute, requires
I6c74938883376ec17f3790678b435585083a440f in core. However, with or without
that patch, the following also works:
{
...
"attributes": {
"Parsoid": {
"Modules": [ ... ]
}
}
}
Bug: T133320
Change-Id: I20f641a1ff032a6da3549b01dfaf8f4cf1eb5071
The `typeof` attribute is a space-separated list. Use consistent methods
to test for membership in that list.
Try to set a good example by using ::addTypeOf() in general to set the
value of the 'typeof' attribute, except when we are transferring values
from one node to another.
Move the *TypeOf method to DOMUtils from DOMDataUtils, eliminating some
duplication. Create Wikimedia\Parsoid\Ext\DOMUtils so that extensions
can use these methods as well.
Change-Id: Ib2ef827ef1cf5e31a1ef0a5034cdd3d9a0212bdb
Since we aren't excluding the other two, MissingDocumentationPublic and
MissingDocumentationProtected.
The stubs could be useful if we ever expanded on what these functions do
and doxygen probably gets the information from here instead of the type
hints?
Change-Id: Ie18c4f00ceca8f06b9c0f0a3359cb4077892f97d
This patch also adds a test case that was missing before. If a
follow="…" is followed by another, normal <ref>, the internal key
(a.k.a. $this->refSequence) is not incremented. This was the case
before, just not covered by any test.
Change-Id: I102d1e67a6918017acc7e4a4663b08c828d101a6
CI already ensures that VisualEditor is loaded alongside Cite, so
the defensive check in the code isn't needed; ext.cite.visualEditor is
defined statically, it's just injected into the page dynamically in the
VisualEditor code handling VisualEditorPluginModules.
Bug: T232875
Change-Id: Ie5e096feca92f9c3ef13c732f3f1ae491e2b7d03
This change does have two effects:
1. Instead of prepending a newline individually in every possible
code path, we do it one time at the end. But only if there is
something in the output. This does not change anything, as proven by
the unchanged parser tests.
2. I removed the newline between the <h2> and the generated
<references> element. Note that both these elements are created in
the same method, next to each other. So there is no way this can
influence other wikitext. Unfortunately this code path is executed
only when using the *preview* function, and impossible to be covered
by parser tests because of this. However, it's covered by unit tests.
This refactoring is motivated by, but not required for T148701.
Bug: T148701
Change-Id: I6691c70f8e3fa3f21e2d11035bed9cdc2dc87093
Previously the reflist was added at the end of the last line of text,
which messes up paragraph wrapping (as seen in many test cases), and
generated invalid HTML when the last line was a list item (T148701).
(second try, previously reverted in 8c933d03c5)
Note this affects only pages where the <references /> tag is missing,
and the references section is auto-generated at the very end of the page.
Bug: T148701
Change-Id: Ib2101346434a4e317b5fc7379215b60c7020cb2b
This will be called by the ExtensionRegistry in core for extensions
that are Parsoid-compatible. The set of registered extensions is part
of the SiteConfig, which bundles all the configuration for a particular
Parsoid instance.
In addition, renamed some classes to make things clearer:
`ExtensionModule` is the thing which is registered; it bundles a
number of `ExtensionTagHandler`s, `ContentModelHandler`s, and DOM
processors (which don't have a proper interface yet). There are a set
of core handlers, which include wikitext, JSON, and a few extension
tags (<pre>, <nowiki>, <gallery>).
Change-Id: Iadbeb378bacb09264a4b1d3ee430a914eec23e48
* We only had a htmlToWikitext API method whereas we have been
trying to stay in DOM land all along. With this change, extensions
can use the intuitive domToWikitext method when they are dealing
with DOM nodes.
* Renamed WTS's serializeHTML method to htmlToWikitext and added
a domToWikitext method there as well which ParsoidExtensionAPI uses.
* Turns out that <ref>s were converting DOM to HTML and then using
the htmlToWikitext method. I switched it use the domToWikitext
method. However, turns out WTS requires a <body> element for its
top-level method!
For now, while we figure out if that can be changed to be more
lenient, added an internal DOM -> HTML conversion in the
domToWikitext method. When we fix WTS, this DOM -> HTML -> DOM
roundtrip can be eliminated.
Bug: T242746
Change-Id: I340d5a363e0d1b8ed6d0ffb0234315e6d9523a76
This is cleaner and less prone to subtle errors since it forces
extension developers to explicitly choose the more performant version.
Bug: T242746
Change-Id: Ia25bc3ae261b43dba97d369940065254faacdd80
Instead of 'fragmentOptions' and 'html2wt' for extension tags,
embed them as 'wt2html' and 'html2wt' components of an 'options'
property.
Bug: T242746
Change-Id: I4cf32a70ec76a415a98b68eef548206f8b917168
Previously the reflist was added at the end of the last line of text,
which messes up paragraph wrapping (as seen in many test cases), and
generated invalid HTML when the last line was a list item (T148701).
Bug: T148701
Change-Id: Ifc873fc913e717026d80d54b570c594d1073fb42
This removes a few tiny pieces of code, and a large chunk related to
incomplete follow="…" attributes (see T240858). It turns out we don't
need to insert elements at the top of the ReferenceStack::$refs
array, because this array is reordered anyway in
ReferencesFormatter::formatRefsList()!
Incomplete follow refs don't have a number, and are ordered to the top
because of this, as before. This doesn't change with this patch.
Change-Id: I43036420be22feb8f0f287d9ccee2afd317df2a9
* Added DOMDataUtils, WTUtils, and Util for use by extension
developers. These classes might acquire more functionality in the
future based on usage and need.
* Added PHPUtils for a single helper to work around a GC bug in PHP.
Once we move on to a newer version of PHP where this is fixed, we
can get rid of this class and helper.
* These classes proxy the various helpers used by currently ported
extensions. For reasons of coherency, the set of helpers in these
classes are a superset of what the extensions use.
* Updated references to the other helpers to use these classes
* Since DOMUtils or DOMCompat are not Parsoid-centric, it feels safe
to provide extensions direct access to those utils classes. We could
consider moving DOMUtils to the Core/Utils namespace if appropriate.
* In one case, I replaced the escapeNowikiTags helper that the Nowiki
"extension" used with an inlined preg_replace.
* TODO: Add unit tests to ensure these utils don't break!
Bug: T242746
Change-Id: I9e733f4ddd6fca8ce13c2957a7d0065d80f7ae9a
* The functionality looks effectively identical to inlineContext
and everywhere inPHPBlock was inspected, inlineContext was also
being inspected.
* Cite's use of this flag is a hack to get desired bacward compatible
behavior but that is a hack no matter what we call the flag.
Change-Id: I3c62590b9bfda224897bb85b18d96c072f3d74ef
* At this point, DSR is a first-class Parsoid concept and
extensions will need to use this as well. So, make it part
of the Core/ namespace to capture high-level concepts that
might be used outside Parsoid itself.
* Move ParsoidExtensionApi to the Ext directory since that is
where it best belongs.
Change-Id: If824c4af9e2f8d658f1cb726cbd837222b60790d
The isModuleRegistered() method was introduced a few years ago,
when the load order in ResourceLoader was undergoing a change.
It used to be that hooks like were run first to register modules, and then
wgResourceModules was registered afterwards. This was reversed to disallow
mutating the config at run-time from foreign modules and to allow better
caching and error detection.
It's been several years since then, so this redundant check is no longer
needed. ServiceWiring.php in MW core for ResourceLoader always processes
config and extension.json first before this hook is called.
Bug: T247265
Change-Id: I466f1fa70b8f0e9fe5e8e8df90bb0001b3329b87
* Added API method to let content-model extensions to add metadata
to <head>.
* The title API methods seem legitimate
* But, the newAboutId helper is suspect -- currently only needed
by Cite. Explore if we can eliminate the need for this helper.
* This eliminates a few more Env use sites from extensions.
Bug: T242746
Change-Id: I0e982d4be173f7d49df19467fbf49c11d428e650
* Cite (or other extensions) don't need to explicitly load/store
data attributes from html attributes to/from the data bag held
separately from the DOM.
Bug: T242746
Change-Id: I4a52be2b06ccfe53d0cf81987af12a1d139fef4c
* Presumably, extensions would benefit from having access to the
wiki config via SiteConfig.
* Yet to figure out if extensions need access to the page config.
* But, with this change, extensions don't need $env when all they
need is access to the wiki and page config.
Bug: T242746
Change-Id: I88736f882f185ee9376b73f7e4bb0b2bd318bb1a
* This seems to work and also will make the job of keeping extensions
free of DOM state easier.
Arlo clarifies that this wasn't necessary since f7594328 and could
have been cleaned up there.
Change-Id: I96edaa5b2743f1ce0d8596acfdc59035491541cb
* $env was unused in extension DOM post processors. So get rid of
that since we are already in the process of removing $env access
to extensions.
* html2wtPreProcessor is currently unimplemented but there is WIP
code in Parsoid/JS that can be revived at a later point. No need
to pass $env here as well.
* In both cases, pass $extApi so they can access any necessary
helpers or state provided by that API object.
Bug: T242746
Change-Id: I1d1544af817d03e01a569e6aeaeed0d6c3058fc0
* Proxy all accesses to the santiizer via appropriately named methods
in the ParsoidExtensionApi interface
Bug: T242746
Change-Id: I9d3d98639bb98b4abe404139786517591323d61d
* Remove use of $env from ReferencesData and RefGroup by
providing high-level helpers in ParsoidExtensionAPI.
- Given a fragment id, provide helpers to fetch fragment DOM
or fragment HTML
- Fetch the URI for the current page (being parsed)
* There is still a lot of subtle knowledge Cite has about
how data-parsoid and data-mw attributes are held off to the
side in a bag and all the pp* and load/store manipulation
of those attributes. It would be an interesting exercise
to purge this implementation of those notions OR figure out
high-level concepts that we document as being part of Parsoid
reality that we'll forever support.
Bug: T242746
Change-Id: I29ff154f2f17123b9756dfd2f3b422f0b30222b1
* In this patch, toDOM, fromDOM, and DOM postprocessor extension
methods all get a ParsoidExtensionAPI object. These API objects
are constructed at the appropriate times in the wt2html and html2wt
pipelines.
* Got rid of direct references to SerializerState from fromDOM
methods in extensions.
* Exposed generic serialization and wikitext escaping methods
in ParsoidExtensionAPI for extensions to leverage. The implementation
of these methods is partial and only supports current usage
of extensions in Parsoid's repo. This will need to be fully
fleshed out going forward.
* Stopped exposing wt2html options in toto and provided more specific
convenience methods.
* Reduced direct access to the Env object in a few more places.
* Cite has code to inspect embedded HTML in data attributes of a node.
Moved this code out of Cite into ParsoidExtensionAPI which reduces
knowledge that extensions need. Unlike the other cleanups, this one
is more of a convenience method since this code only requires
knowledge of a publicly published spec. But, nevertheless an useful
cleanup since it simplifies Cite's complexity just a bit.
* More followup work is needed.
- before/after methods should be eliminated in favour of a config flag
that implements the inline/block layout option. Once this is done,
extensions will no longer need direct access to the SerializerState
internal object.
- Env exposure should be reduced.
- Provide access to Sanitizer via ParsoidExtensionAPI instead of
needing extensions to directly import it.
- It should be possible to eliminate the need for extensions to know
about DSR / DSR-shifting and do it automatically via some high-level
conceptual flag.
- It might also be possible to infer source offsets directly via args
instead of passing that explicitly.
- Should we provide a convenience helper class with access to all the
src/Utils/* methods?
Bug: T242746
Change-Id: I7ffb5aa52a84854a9d363a0e8f1ce650241f1c41
This patch is mostly moving code around without changing the behavior.
Exceptions:
* The ErrorReporter creates a <span> container. This was previously
parsed. The only benefit might be error checking and escaping. Rather
pointless. The code just created this HTML. With this patch, it is not
parsed any more. The unit test reflects this change. The output in
production will not change, as the parser tests show.
* Parsing of the message key (to detect it's type and id) is simplified
a lot, using explode. With this the code can, in theory, support more
types.
Bug: T239572
Change-Id: If2fe5f55db46dfc7e0ce445348608bef00bec64e
Perform the validation in validateRef, and display a new error message for
broken "follow" refs. This changes existing behavior, where broken folow
ref content is arbitrarily displayed at the top of the references list and
no error is rendered.
Thanks to weasely wording, the new error can later be reused for "extends"
errors.
Bug: T240858
Change-Id: I506e4dcd1151671f5302ecd99581145d979d8124
This exception was introduced very late in the patch I38c9929. It
already caused trouble. This here is essentially a revert. It restores
the previous behavior where this edge-case was silently ignored. The
worst thing that can happen is that appendText() creates an incomplete
entry in the $this->refs array, which will be rendered at the end. The
user can see it then.
As of now we are not aware of a code path where this would even be
possible. Still this does make the code *more* robust by not making it
explode, but give the user something they can work with.
Bug: T243221
Change-Id: I2e2d29bbd557090981903fcc2ece8796fafa4aa4
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
"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
Allow a ref with `name=""` for backwards-compatibility.
Partially reverts I07738cce2641026dfaa92ba263ed6f9834be0944
Bug: T242437
Change-Id: Iaed2d1c41be377a4961aff39838b0965f6c00616
The goal of this patch is to not change any behavior, just make the
code less nested and less complicated.
Change-Id: I89170960ffbf61f57e245adf097f3e8d8196bbce
The difference between the two is that isOK() only reports "fatals",
while isGood() also reports "warnings" and "errors". I believe we
*want* to report all of these the same way.
Change-Id: I3be832c5db7aba3c03bd2ad8cfbba42362c093fd
A fun edge case where `name=""` fools both validation branches after
a references rollback, and triggered a LogicException. Stop these
freak refs.
Bug: T242437
Change-Id: I07738cce2641026dfaa92ba263ed6f9834be0944
It's possible to nest <references> by using tricky constructs like the
{{#tag function, and this breaks our rollback logic. Try to show normal
output, otherwise show an error.
Includes regression tests.
Bug: T242437
Change-Id: I33e497cdf8508ce7ccb7f0f315c00af5eee47d0e
This error happens only when previewing an edit, because some of the
validation in Cite::validateRefInReferences() is disabled in preview
mode. Unfortunately this codebase was never properly tested in preview
mode.
This patch is intentionally so small to make it easy to backport.
Tests will follow.
Bug: T242434
Change-Id: I5e529b7227598ab2acc624c90a0cb5d09b3f5452
* 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
Since I3db5175 the ParserCloned hook handler does not rely on cloning
the Cite object any more. There is no cloning any more. This is dead
code and we could remove it. Just to be sure I propose to keep the
method, but let it throw an exception.
Bug: T240248
Change-Id: I2057ea652ca25f4c7031c28a6e713671738f5e22
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
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
Before, this regular expression was looking for incomplete wikitext
like this:
<ref>unclosed
<ref>closed</ref>
With this change, wikitext like this will trigger the same error:
<ref>unclosed
<references />
incomplete</ref>
This should be much, much more rare. But I feel it's reasonable to mark
this as an error, instead of just rendering the broken inner tag in
plain text.
This patch also replaces `.*?>` with `[^>]*+>`. Both do the exact same.
Instead of doing an "ungreedy search for the first possible closing
bracket", which might cause backtracking, the new syntax consumes all
non-brackets before expecting one. This is guaranteed to never backtrack
(guaranteed by the extra +), and potentially faster because of this.
Change-Id: Ic76a52cd111b28e4522f095ce3984e3583f602c1
The concept "key" already exists in the structure handled by this
function, so to have a $key which means something else was distracting.
Change-Id: I91a76edbb42a1ab6514bc706b75ab89f78539fa5
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
This partly reverts Id7a4036e64920acdeccb4dfcf6bef31d0e5657ab.
The message "cite_section_preview_references" says "Preview of references".
This line is not meant to be part of the content, but an interface message.
It should use the users (interface) language, not the content language.
Change-Id: I1b1b5106266606eb0dfaa31f4abd3cee9ba92e8c
This simplifies as well as fixes a series of issues with this regular
expression:
* Before, the wikitext `<REF><REF>` would not trigger the error, but
`<ref><ref>` would. Parser tags are case-insensitive, but the error
check was not.
* Before, the wikitext `<ref><ref name="<">` would not trigger the error.
That's a valid name. The error check should not stop just because it
found a `<`.
* Both the old and the new code do *not* fail with the wikitext
`<ref><ref</ref>` where the inner `<ref` does not have a closing `>`. I
was thinking about changing this, but figured it might be used as a
feature.
* The old code was not able to properly understand HTML comments,
<nowiki> tags and such that contain a line break. That caused
inconsistent and confusing error reporting in some cases, but not in
others. This change *reduces* the amount of errors this code produces.
* The old code was looking for "SGML tags" with names that could be
anything, not just alphanumeric characters. This allowed for strange
edge-cases like `<ref><>><ref></>></ref>` that have not been reported,
but should be. This change *increases* the amount of errors. However,
relevant edge-cases should be extremely rare.
Note the ++ avoids backtracking, speeding up the regex.
Change-Id: I0c61a245f4f743871b4cad886ce239650af2b37c
We never access Language directly, so proxy its method instead of
returning the full object.
I believe I've found a bug, but not fixing here: the footnote body
numeric backlinks like "2.1" behave as if they were decimals rather
than two numbers stuck together with a dot. So they are localized
to "2,1".
Bug: T239725
Change-Id: If386bf96d48cb95c0a287a02bedfe984941efe30
I was able to track this code down to I093d85d from 2012, which was done
right after the ParserAfterParse hook was introduced. I believe the
redundant code path was left to keep the Cite extension compatible with
old MediaWiki versions that did not had this hook yet.
I also noticed this code path is most probably entirely redundant with
the current version of MediaWiki. The *only* thing this code does is
blocking the ParserBeforeTidy hook from doing the same thing a second
time if the ParserAfterParse hook was called before. But it does *not*
block any other compination, e.g. if the two hooks are called the other
way around, or the same hook twice.
In core, it looks like it is impossible for the ParserBeforeTidy hook
being fired without the ParserAfterParse hook being fired before. If this
is true, this is in fact dead code.
Change-Id: Iacf8b600c7abdeaf89c22c2fc31e646f57245e47
Encapsulate the language interfaces, this will be used to replace
global wfMessage calls in future patches.
Change-Id: I7857f3e5154626e0b29977610b81103d91615f65
This makes it obvious that our function isn't sensitive to the input value.
Also rearranges a string concatenation to make the element wrapping clearer.
I probably should have switched to the HTML class here, but I'm not sure what
the advantages would be.
Change-Id: Ife3424ce68588f73f168b10e63e6cd81c4a60084
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
* Don't use string comparisons to compare numbers.
* Avoid isset() for variables that are guaranteed to exist.
* Inline two small "gen…" functions that are only called once.
* Move the fallback code path out of getLinkLabel(). Before it was
always called. Now it's only called when needed.
Change-Id: I42073f57f21d32c7936954da776ef3a393410020
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
Pass the full ref structure from ReferenceStack to FootnoteMarkFormatter,
to give it control over the final rendering. This is aligned with how
the FootnoteBodyFormatter directly scans over groupRefs.
Change-Id: I3294fd9366f01daa4250a5d481f4adbae84c72b1
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
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
This patch does two things:
* Add strict PHP 7 type hints to most code.
* Narrow the interface of the checkRefsNoReferences() method to not
require a ParserOptions object any more.
Change-Id: I91c6a2d9b76915d7677a3f735ee8e054c898fcc5
This fixes a FIXME I left in the code. Previously, I just stripped the
closing </li> to make sure the nested <ol> is *inside* of the <li>.
This relies on (Remex) Tidy to clean the incomplete HTML up.
This patch remembers the stripped </li> and adds it back.
This also makes sure the nested <ol> is closed, even if it was the
last element in the data structure.
Notice how this does not influence any test. I find this a bit
confusing. It looks like (Remex) Tidy is executed, even if the tests
are not marked as "html/php+tidy".
Bug: T237241
Change-Id: Idb804df46dc24406d6bba40414675b6ff4812d48
The "no key" error should have been unreachable, but I'm afraid that
null `$text` and empty string `$text` were reporting slightly different errors.
Unfortunately, we still have to care about `$text = '0'` because PHP, so
the expressions don't reduce to `if ( !$text ...`
Change-Id: Id1028611ec3bc462dca413f31f7f59637bd7cc7b
There was a call in the API that was *not* using normalizeKey(). Now
that the API is gone, we can inline this.
This patch also contains a bunch of cleanups that might already been
resolved in the previous patches.
Change-Id: Id3767b5830268c8cfe9c10efabfa4a31e9dafeb8
Forked from Icd933fc983.
Bugs and unimplemented features are documented as TODOs in the parser test
fixtures.
Bug: T237241
Change-Id: I9427e025ea0bcf2fa24fd539a775429cc64767cc
This API was never used in Wikimedia production, and would have caused
performance problems. Removing the dead code will simplify our refactoring.
Bug: T238195
Change-Id: I7088f257ec034c0d089e0abdaa5a739910598300
This is motivated by I9427e025e, which demonstrates that the existing
logic was hard to integrate into. There's a lot of redundant expressions
which make the function difficult to read, and code paths which have
less effect than they appear to.
Change-Id: Ida9612d1457f2593647b8fc02930d2e9ae824814
We realized the trim() are not needed. This does not leave much behind
in the existing refArg() method, except that it checks for unknown keys.
I tried a few strategies and ended using the pretty new possibility to
have keys in list(), as well as use [] instead of list(). Both is
supported since PHP 7.1.
Change-Id: I569bfa14e68b64402519bd39022c197553881dde
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
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
That was annoying me. Since we're passing a bare list, alphabetical
order helps make the code and tests readable.
Change-Id: I6384094e429e0e2a6fa810fdc28ae0643a0ccf7c
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
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
Validation logic can be split from arg parsing, default values and
other side-effects.
No behavior was changed.
Change-Id: I2d9904b7631d0d6be13e0aaed0106f186d388c4f
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
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
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
* 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
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
One of the test cases was duplicated, but a lot of the possible code
paths never had tests, including the happy code path!
I found this issue while trying to rework some of the more confusing
loops in this codebase. These changes are still part of this patch. All
loops still do the same as before, but are (I hope) more readable now.
Bug: T238187
Change-Id: I85baeadd9b149025a14c7522bcc4182339c66972
… 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
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
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
It is about time to implement a generic fix for T214994 which will
capture the Cite-specific scenario we are tackling here in bits
and pieces. We have fragments of a T214994 solution spread in
Utils/ and here while tackling shiftDSR and convertOffsets
for Parsoid/PHP. So, someone needs to take a look at all these
pieces and implement the necessary abstraction.
As part of that, maybe we also need to revisit our embedded HTML
data-* representation so there is a uniform way to inspect these
attributes rather than every semantic element (template, extensions,
lang variants, media) choosing their own custom embedding scheme
which makes discoverability harder and error-prone.
Change-Id: Ifd19d2d8d20dbd0dda0fa1cc338a07afb37c4213
This managed to catch a few bugs where there is likely coverage gaps.
The regenerated plugin list again comments out a few that generated too
much noise but may be worthwhile follow ups.
Change-Id: I746abd0f1406b4b7ffa497afdad3939fe437c25d
* This is an instance of a bigger issue that we need to look closely
when we are integrating Parsoid with core parser.
Bug: T235656
Change-Id: I3d652727293461c7968e83be8994ba0572bae8e4
* Though this doesn't immediately affect anything, it just makes
usage a bit more consistent.
* A followup patch that fixes gaps in shiftDSR code will now be
able to reference the html property as dmw->body->html to match
html2wt usage.
Change-Id: I9dfcd9d40205f6e64e139bf3f75a322915af3232
* When a page is missing an explicit <references /> tag, we insert
an implicit <references /> tag and assign it a zero-width DSR with
a starting offset equal to the length of the page. However, now that
we have byte offsets, that should have been strlen, not mb_strlen.
This was causing incorrect DSR assignment on this implicit tag
and causing trailing newline selser diffs on these pages.
* Debugged on this reduced test case: "* a – b <ref>x</ref>\n\nc\n\n"
and comparing selser trace and then DSR offsets on the DOMs.
Change-Id: I8aebf307197935259df78251fb4a26c593f29603
Tim Starling has indicated in couple different places that end(..)
is not preferred and he had implemented a private version of lastItem
in the PEG grammar code whereas PHPUtils::lastItem was recommending
use of end(..).
In this patch, I moved the implementation from the grammar to PHPUtils
and replaced end(..) with PHPUtils::lastItem in a number of places in
the codebase. We should discuss whether we want to use this helper
everywhere.
Resolved a couple of PORT-FIXMEs in the bargain.
Change-Id: I837f2a98003df8ab7dbdf9af045e17bdd6e27799
* We could potentially also exclude regexps for node name checks
* A few additions looks like could potentially have caused subtle
failures in edge cases.
* Unrelated changes: Used # regexp terminator in a number of regexps
to eliminate escaping of / character.
Bug: T231980
Change-Id: Ie2451349684c248d93e064e3e7009d0d2d60acf3
The extension handler expects a `null` for this, `false` is an
indication not to use the toDOM handler.
php bin/parserTests.php tests/citeParserTests.txt --filter "CircularRef" --wt2html
Change-Id: I849a9aca1133f8a793c9d77e05f192a6af5d78f9
Introduce PHPUtils::arrayToObject() which duplicates the array before
converting it to an object. Workaround for
https://bugs.php.net/bug.php?id=78379
Bug: T228346
Change-Id: If9ef35e9e5183117025bc9cd705b695f270aa244
Two classic PHP errors
- string '0' is a falsy value in if conditions and so we need an
explicit === '' check for empty strings
- arrays need to be passed by reference to capture modifications
in callers.
Change-Id: I07d0e39c44a923ac1faeb2de01433e951c3de914
* Don't unconditionally run fromHTML and before handlers without
checking if we have a native extension handler.
* Remove unintential implementation of `before` in Ref.php
* Hybrid tests for Cite now passes again.
* Discovered while running native parser tests and isolated those
crashers to this.
Change-Id: I45b48b595a5aee2b8b8d00b4ebcf73a5ea7bc8a3
Restructure ExtensionTag as an abstract class with default do-nothing
implementations of all methods. So instead of Translate and LST
not implementing ExtensionTag::toDOM, they inherit the default
implementation which returns false, and that has the same effect.
The intention is to move SerialHandler::fromHTML and SerialHandler::before
into this framework as well. Every "optional" method should have a default
implementation in the base class which returns false.
Change-Id: I0ad5c714601c0cf0b5189d4d282c67c6b53fc760
Passing srcOffsets which don't actually correspond to actual regions of
the source wikitext cause problems in the token offset conversion code.
Instead, parse the wikitext as itself, then adjust the TSRs in the DOM
tree.
Since Gallery isn't ported to PHP (yet), update the
automatically-generated Gallery/index.php. The newly-added
ContentUtils::shiftDSR() was ported, however.
Change-Id: I28f3d3398930733ae2bcf9759e49c45f93bc7190
* Now that we are in sync land, we don't need to catch exceptions
and log error messages at multiple places. Let them bubble up
to the top.
* I noticed this was actually getting in the way of debugging because
with $env->log unimplemented, I was only getting very generic
failures instead of the root cause that was being suppressed
and unlogged.
* There are still a couple of places where we have generic Exception
catching in place where it does make sense currently. For example,
we aren't interested in what caused a templatedata fetch to fail.
We simply fall back to regular serialization - the rationale here is
that it is better to emit a transclusion without the preferred formatting
(but syntactically correct) instead of losing the edit altogether.
* Minor unrelated fix in Cite/Ref.php: Use !isset() instead of empty()
Change-Id: Iebff6f37dcd8278185c4a74b72a99b528efa20ff
* Source offset fixes: followup to 31d356a5
- there were instances of $tsr[0] and $tsr[1] that hadn't
been converted over to $tsr->start, $tsr->end
- removed dead code
* Cite fixes: followup ot 005176a3
- Fixes array / object mixups
- Bug fix
* html2wt/WikiLinkHandler fixes
- Protect access to missing properties in data-mw opt list
* Other assorted fixes
- Added missing typehints and improved doc types
- Simplified some code patterns
- Cast extension attributes to object since that ends up
in data-mw which is a stdclass object.
Change-Id: Idd04b0d3819be3660823047a90330fd1213388cf
* All wt2wt, html2wt, and all but one html2html tests pass in
hybrid mode when entire html2wt code is run in PHP
Set "Serializer: true" in the html2wt section of phpconfig.yaml
* The single failing html2html test is a <gallery> test which is
presumably related to the unported <gallery> extension code, but
not sure. Not investigating it now.
* Update Parsoid Extension API to provide access to extension source
without exposing internals.
Change-Id: I6d6e21ad2324acfc4306b32c9055d6c088708c48
* unwrapFragment had a somewhat unusual behavior which could be
a source of bugs while reasoning with it.
If undefined, it's default value is true which is contrary
to how we think of undefined.
* Flipping the polarity of the flag to sealFragment makes the
semantics easier to reason with and where !empty(..) applies
more naturally to it.
Change-Id: Ia50cba345f37e815e5f5f95abb452c8eefcf9011
In PHP, DOMNode#getAttribute() return '' if the attribute is not present,
not null. Audit our uses and try to either explicitly use `|| ''` (which
will ensure that PHP behaves the same was as JS) or use `hasAttribute`
to explicitly test for the presence of the attribute.
Changes have also been ported to PHP from JS.
Also added src/Wt2Html/PP/Processors/AddMediaInfo.php which was missing.
Change-Id: Ie1ae1df88e4fca70daf97b6f720f28014ebc99ed
This applies the JS changes from the following recently-merged patches:
6679c3bf Protect data-object-id attribute
d4e76d5b Fix new linter category to enable code work with templates
e567db8d Tweak storeDataAttribs to suppress DOM nodes in data-parsoid.tmp
16603953 Fix setting dsr on body for genTest
3a84a9dd Fix stashing data attributes for mw:StartTag
22c4a19a Remove redundant dataParsoid call
ed7b0ba0 Fix crasher in newly added linter category
505a357b Linter.js: Add new function to detect the use of links in links
8885b20e Move redlink updating into lib/parse.js
ccfce23d templatedepth is either an int or false
6d1571bd Move language conversion work into lib/parse.js
5a89c7de Avoid serialize/parse of data attributes when treebuilding
021d9958 Rename `document.env` to `document.bag`
c03ba494 Use XMLSerializer on both PHP & JS side in the DOM pass test script
e0c3cca9 Use env.createDocument in lib/api/apiUtils.js
550d3d71 Use a bag-on-the-side implementation for node data
f8de8b25 Add bin/inspectTokenizer.js
db704eea Add ability to splice a PHP transformer into the pipeline
a8be3ad6 Fix crasher in cite extension from accessing data after it's stored
2874f200 Simplify and clean up stops usage
6368265d Add some strategic isElt guards
5ae9553f DRY out transform test runners + tweak genTest to enable that
b0f2adc6 Assert that the .dataobject isn't touched after storing attrs on a node
1ce6a98d Skip separators when looking for the next th/td
Change-Id: I6a66ecb061e7ee7ed53feba1895dd315d9324715
* This initialization lets us do a git log --follow and follow
git history for that file across the js -> php port boundary.
This works because git uses content hashes for objects and
the copied code in the new .php file will have the same content
hash as the .js file.
* The following directories were skipped
- ./lib/config/baseconfig
- ./tests
* The following JS files were skipped
- ./lib/utils/promise.js
- ./lib/config/wmf.sitematrix.json
- ./tools/sync-baseconfig.js
- ./tools/sync-parserTests.js
- ./tools/fetch_ve_nowiki_edits.js
- ./tools/fetch-parserTests.txt.js
- ./tools/fetch-wmf-sitematrix.js
- ./tools/compare.linter.results.js
- ./tools/fetch-revision-data.js
- ./tools/fetch-wt.js
- ./tools/regression-testing.js
- ./tools/build-langconv-fst.js
- ./bin/server.js
Change-Id: I0b22057c23b72795aebbd66e3abcb627c6858ef3
