While in many cases the class will never be sub-classed, it's easier
just to always use static:: and not worry about predicting which
classes might have problems in the future.
Change-Id: I23072a1701b5acf62bb3379a877de97627d8fcf3
Change the order of checks to ensure that we have at least one comment
before we try comparing ranges, to avoid issues with empty headings
having collapsed ranges. It should be a tiny bit faster this way, too.
Bug: T304377
Change-Id: I59ad30cfc075dcec882e048d2d199744efec2114
Also fix a bug where headings would be ignored while checking for
comment frames. See task for detailed explanation.
Bug: T303396
Change-Id: I6495826b4b050ea80680e0798ac6ab4497a7c09e
Phan can analyze them now and reports some issues with types.
* Add some assertions on types where we're sure that we're using an
Element or non-null, but Phan can't prove it
* Fix incorrect type hints on getFullyCoveredSiblings() and
getCoveredSiblings(), luckily it was harmless
Change-Id: I8cc12450378efa7434c4d66882378b715edd4a70
`tagName` is only defined on Element, and it returns its tag name.
`nodeName` is defined on Node, and it returns the tag name for Elements,
and a string like '#text' or '#document-fragment' for other types.
We were using both, which made it harder to reason about what types
we're dealing with.
Change-Id: I8e621e5872bdf78c84ec553cfbfcdbf0192f0589
It is friendlier for static analysis tools like Phan, which can't
infer anything from the `->nodeType === …` checks, and we were already
using it in most places.
Fix newly revealed Phan failures (and one unneeded suppression).
Change-Id: Id789f05e16a210f7ba22ca7514587c392fac0741
Added in 76289cdf73,
should no longer be needed since we switch to Parsoid's
HTML parser in 3e6ab2c4d2.
Change-Id: Ic0b7ed8089b71f2338e604f68d547759e069f0b2
Also special-case thumbnail wrappers generated by
MediaTransformOutput::linkWrap, for compatibility with
TimedMediaHandler.
Bug: T301427
Bug: T302296
Change-Id: I7f48d8b2261507c5a33526c54109f5187d062ed3
Previously, we required a signature at the end of the comment.
This was a pretty rough heuristic that did not correctly handle
many comments that we would consider entirely properly signed
in CommentParser (e.g. comments wrapped in formatting like
<small>…</small>, comments with a post-scriptum or in parentheses,
or comments generated by various templates).
Now we process the user input using the same code that adds reply
links, and only add a signature when we detect that there really
isn't a signature (including template-generated), or if the signature
is in the wrong place and would result in the reply link showing up
in the wrong place as well (not at the end of the comment).
Bug: T278442
Bug: T268558
Bug: T278355
Bug: T291421
Bug: T282983
Change-Id: I46b6110af328ebdf93b7dfc2bd941e04391a1599
Also, in ThreadItem::getSinglePageTransclusionTitle(), we don't need
this terribly complicated method.
Change-Id: If02c09aaa2f4dd66b2bc253a1edec4ea107564ee
When we encounter a node that doesn't represent comment contents, e.g.:
* a [reply] link we inserted (T297034#7641334)
* an {{outdent}} template (see changed test case)
…we should ignore it together with its descendants (like in
Parser#nextInterestingLeafNode), instead of processing descendants
and possibly detecting comment contents in them.
Follow-up to 8de940b587,
72b9c2c6f5.
Bug: T297034
Change-Id: Ib2fa40c5fa389572b0e88ef558728fa06e3621b0
In PHP, use DOMCompat::getClassList(), provided by Parsoid.
In JS, use `.classList`, available in all supported browsers.
This may fix some bugs where we were incorrectly checking for exactly
one class. The change in isOurGeneratedNode() is needed for
Ib2fa40c5fa389572b0e88ef558728fa06e3621b0.
Change-Id: Ia28d31678fd3d617b69280c4b7857755300fa515
Reimplement getFullyCoveredSiblings() using compareRanges(), which
checks basically the same thing, but works better and I like it more.
Bug: T297034
Change-Id: I33dc1d088bdee984064315290e378bfbfa830b10
Previously: 569db3603c (2020-06).
Unfortunately we've found cases where the previous implementation
doesn't work correctly, resulting in comments being added to the wrong
pages or page corruption.
Bug: T289873
Bug: T298051
Change-Id: Id867b3005ebc46906d6df852a525fcaec9e6b19b
Usually this isn't a problem, because the comments are marked as
template-generated and we don't allow replying to them. But we had a
special case where we were trying to skip over some invisible
elements, which was causing us to skip into the middle of the
about-group in some cases. When Parsoid sees that, it serializes the
contents twice.
Bug: T290940
Change-Id: I9fe0b8d43ab874ccef371990799f77bfc46bc954
The PHP DOM extension measures lengths and offsets in Unicode codepoints.
Our PHP code used UTF-8 bytes, causing some offsets to be slightly off.
Now it mostly uses Unicode codepoints as well (we're forced to use bytes
in a few places, because preg_match returns offsets in bytes).
In practice, this had no visible effect to the user. It caused the
markers `<span data-mw-comment-end="..."></span>` to be placed at
the end of their container instead of the correct position when the
timestamp contained multibyte characters (e.g. "ź" in Polish); but
the correct position is usually at the end of the container anyway.
In the test cases, the only difference is placing these markers before
a trailing line break inside `<p>...</p>` tags rather than before it.
The patch also accidentally fixes another bug, where element nodes
with no children (mostly <img>) were incorrectly excluded when calling
cloneContents(), because they were treated as if they were text nodes.
Change-Id: Iccdccf1078598f4b62cab96225e9c85a4c0e93ee
Use DOMCompat::querySelectorAll() instead.
CommentModifier::isHtmlSigned()
* Copied the CSS selector from the JS equivalent function.
CommentUtils::unwrapParsoidSections()
* Copied the CSS selector from the JS equivalent function (in VisualEditor).
CommentItem::getMentions()
* Trivial.
This causes Phan to report some more issues, which are also fixed.
Follow-up to 25272e7a4a.
Change-Id: Iaf1222f7114916f2eca19942c3686168899486fd
These changes ensure that DiscussionTools is independent of DOM
library choice, and will not break if/when Parsoid switches to an
alternate (more standards-compliant) DOM library.
We run `phan` against the Dodo standards-compliant DOM library,
so this ends up flagging uses of non-standard PHP extensions to
the DOM. These will be suppressed for now with a "Nonstandard DOM"
comment that can be grepped for, since they will eventually
will need to be rewritten or worked around.
Most frequent issues:
* Node::nodeValue and Node::textContent and Element::getAttribute()
can return null in a spec-compliant implementation. Add `?? ''` to
make spec-compliant results consistent w/ what PHP returns.
* DOMXPath doesn't accept anything except DOMDocument. These uses
should be replaced with DOMCompat::querySelectorAll() or similar
(which end up using DOMXPath under the covers for DOMDocument any way,
but are implemented more efficiently in a spec-compliant
implementation).
* A couple of times we have code like:
`while ($node->firstChild!==null) { $node = $node->firstChild; }`
and phan's analysis isn't strong enough to determine that $node is still
non-null after the while. This same issue should appear with DOMDocument
but phan doesn't complain for some reason.
One apparently legit issue:
* Node::insertBefore() is once called in a funny way which leans on
the fact that the second option is optional in PHP. This seems to be
a workaround for an ancient PHP bug, and can probably be safely
removed.
Bug: T287611
Bug: T217867
Change-Id: I3c4f41c3819770f85d68157c9f690d650b7266a3
For compatibility with Parsoid's document abstraction (Parsoid may
switch to an alternate DOM library in the future), don't explicitly
create a new document object using `new DOMDocument`; instead use
the Parsoid wrapper `DOMCompat::newDocument()`. This ensures that
the Document object created will be compatible with Parsoid.
There are a number of other subtle dependencies on the PHP `dom`
extension in DiscussionTools, like explicit `instanceof` tests; those
will be tweaked in a follow-up patch
(I3c4f41c3819770f85d68157c9f690d650b7266a3) since they do not affect
correctness so long as Parsoid is aliasing Document to a subclass of
the built-in DOMDocument. Similarly, the Phan warnings we suppress
do not cause runtime errors (because of the fixes included in
c5265341afd9efde6b54ba56dc009aab88eff83c) but phan will be happier
once the follow-up patch lands and aligns all the DOM types.
Bug: T287611
Depends-On: If0671255779571a91d3472a9d90d0f2d69dd1f7d
Change-Id: Ib98bd5b76de7a0d32a29840d1ce04379c72ef486
Now it detect signatures generated by en.wp's {{Undated}} template,
and signatures of people who do weird stuff to the timestamps.
Bug: T275938
Change-Id: I27b07f6786ca5433a3c02a5fe68e4716d41401bb
In some cases it would return the parent node, instead of the siblings
it should return.
It's a private method only called by getFullyCoveredSiblings(), and
that method had a bug that cancelled out this one, so everything
worked correctly. But I want to use it elsewhere now and ran into it.
Change-Id: Ic12f007d57a8502a1bea5f0af17b29e9d59093d6
I found this error in our logstash. I was not able to find an
existing Phabricator ticket.
Note how line #348 extracts the last element from the
$siblings array. It uses the function end() there, which
returns false in case the array is empty. $siblings[0] can't
do this but yields an error.
An alternative is to use reset(), which can return false as
well. But that's not really better. Especially not better
readable, I would argue.
Change-Id: Ic90cd2392ede15078ba0d5b4d67b8dc5d05f9bf7
* Ignore rendering-transparent nodes between discussion comments.
* Improve isRenderingTransparentNode() so that <link> nodes
representing TemplateStyles are not considered transparent,
otherwise this would undo ae920b831f.
Using a regexp from Parsoid.
Bug: T272746
Change-Id: I0b3c3251156ba6c4826abf5ba44ea93f80ebc01d
Add yet another tree walking utility: CommentUtils::linearWalk().
Unlike TreeWalker, it allows handling the beginnings and ends of nodes
separately – kind of like parsing a XML token stream, or kind of like
VisualEditor's linear model.
(Add unit tests for this utility. The simple.html test case is copied
from [VisualEditor/VisualEditor]/demos/ve/pages/simple.html.)
Use this utility to stop skipping when we reach either a closing or
opening block node tag. Previously we'd skip over such tags inside
nested "transparent" nodes (like <a>, <del>, or apparently <font>).
Bug: T271385
Change-Id: I201a942eb3a56335e84d94e150ec2c33f8b4f4e0
As a result of 0fc71f60cd, "empty" text
nodes (containing only whitespace) at the end of the comment may be
inside the comment's range, and trying to ignore them caused the
ranges not to match and the frame not to be detected.
Now the code works whether they're inside the comment's range or not.
Add a test case for wrapped discussion comments with HTML comments and
with whitespace.
Bug: T250126
Bug: T268407
Change-Id: I2217ff5a635fd1c9c9e803f46795b1bfb3d17535
Move the code so that we check for "?title=" query parameter first,
because we don't handle this right in the other code path.
Use parse_url() instead of wfParseUrl() because the latter doesn't
accept relative URLs, and we don't care about the other differences.
Bug: T261711
Depends-On: I4da952876e1c3d1a41d06b51f7e26015ff5e34d7
Change-Id: I70fac2b41befd782b0a47a4f726ae748dc0f775d
When a comment ended before the end of a paragraph, the next
comment would begin right there in the middle of the paragraph.
This could result in the detected indentation level of that
comment being incorrect, and replies being inserted in wrong
places, as seen in the 'signatures-funny' test case.
The code moved to the parser was previously repeated twice in
addListItem() and addReplyLink(), which should have been a hint
that something isn't quite right.
Also, fix the code guarding against overlapping signatures,
now that signatures may not be at the end of a comment.
Bug: T260855
Change-Id: Ic26a87642f8a15d5de2f7073d4d8176b299c7f94
