Rename ThreadItem to ContentThreadItem, then create a new ThreadItem
interface containing only the methods that we'll be able to implement
using only the persistently stored data (no parsing), then create a
DatabaseThreadItem. Do the same for CommentItem and HeadingItem.
ThreadItemSet gets a similar treatment, but it's basically only for
Phan's type checking. (This is sad.)
Change-Id: I1633049befe8ec169753b82eb876459af1f63fe8
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
Goal:
-----
Finishing the work from Iadb7757debe000025e52770ca51ebcf24ca8ee66
by changing CommentParser::parse() to return a data object, instead of
the whole parser.
Changes:
--------
ThreadItemSet.php:
ThreadItemSet.js:
* New data class to access the results of parsing a discussion. Most
methods and properties are moved from CommentParser with no changes.
CommentParser.php:
Parser.js:
* parse() returns a new ThreadItemSet.
* Remove methods moved to ThreadItemSet.
* Placeholder headings are generated slightly differently, as we process
things in a different order.
* Grouping threads and computing IDs/names is no longer lazy. We always
needed IDs/names anyway.
* computeId() explicitly uses a ThreadItemSet to check the existing IDs
when de-duplicating.
controller.js:
* Move the code for turning some nodes annotated by CommentFormatter
into a ThreadItemSet (previously a Parser) from controller#init to
ThreadItemSet.static.newFromAnnotatedNodes, and rewrite it to handle
assigning parents/replies and recalculating legacy IDs more nicely.
* mw.dt.pageThreads is now a ThreadItemSet.
Change-Id: I49bfe019aa460651447fd383f73eafa9d7180a92
Goal:
-----
To have a method like CommentParser::parse(), which just takes a node
to parse and a title and returns plain data, so that we don't need to
keep track of the config to construct a CommentParser object (the
required config like content language is provided by services) and
we don't need to keep that object around after parsing.
Changes:
--------
CommentParser.php:
* …is now a service. Constructor only takes services as arguments.
The node and title are passed to a new parse() method.
* parse() should return plain data, but I split this part to a separate
patch for ease of review: I49bfe019aa460651447fd383f73eafa9d7180a92.
* CommentParser still cheats and accesses global state in a few places,
e.g. calling Title::makeTitleSafe or CommentUtils::getTitleFromUrl,
so we can't turn its tests into true unit tests. This work is left
for future commits.
LanguageData.php:
* …is now a service, instead of a static class.
Parser.js:
* …is not a real service, but it's changed to behave in a similar way.
Constructor takes only the required config as argument,
and node and title are instead passed to a new parse() method.
CommentParserTest.php:
parser.test.js:
* Can be simplified, now that we don't need a useless node and title
to test internal methods that don't use them.
testUtils.js:
* Can be simplified, now that we don't need to override internal
ResourceLoader stuff just to change the parser config.
Change-Id: Iadb7757debe000025e52770ca51ebcf24ca8ee66
(suggested by PhpStorm)
composer.json:
* Document required PHP extensions
Parser.js:
* Remove incorrect param documentation
* Fix some typos in comments (missing parentheses)
CommentParser.php:
* Fix some typos in comments (missing parentheses)
ImmutableRange.php:
* Remove unused property
* Add a `throw` to indicate that code path is unreachable
SubscribedNewCommentPresentationModel.php:
* Add missing `return false`
CommentParserTest.php:
* Remove unnecessary pass-by-reference
CommentModifierTest.php:
* Remove unused variable
CommentParserTest.php:
* Don't construct Element objects directly. PHP's DOMElement allows
it, but Parsoid/Dodo's doesn't, and we use the latter for static
analysis. This generates all kinds of confusing warnings.
Change-Id: Ia9598ebea0e99830dd485296e94a9d96acc4b258
This is now deployed on all wikis, and going forward I don't think
we need to make this configurable.
Change-Id: I231976267ba6cdfeec622efaa15983a84c330649
This code previously ensured that the fragment identifier linking
to a section was only included if all events had the same section.
It doesn't actually seem worth the effort, since we handle scrolling
to the highlighted comments client-side anyway.
And the links were not quite correct, because we didn't parse and
strip the section title as expected by built-in Echo events. Just
use Echo's code for this.
Depends-On: Idb3a87fd18330f90a8cdc1276994d54288e17b28
Change-Id: Icae0d3654dd02109337ff8737b16f55bbd514f43
The global function wfWikiID() is deprecated since 1.35 and it's usages
should be replaced with WikiMap::getCurrentWikiId().
Bug: T298059
Change-Id: Ie878a5479b7427e9ffab7d7f92ee2802997e3161
Better describes that we are checking the editor used to make
the edit, rather than descibing some virtual "location".
Change options to 'discussiontoolsapi' and 'any'.
Change-Id: I3024517e70ed61c738e4bf46a2ac7b58c975d98a
According to Daniel it only worked by accident, and stopped working
after de63ad823abe:
getPageByReference() used to do an opportunistic lookup by ID when given
an instance of PageIdentity -- which is correct for EventDispatcher,
but problematic in the general case, causing T296063.
The correct thing to do here is to use getPageById(), since the canonical
association between revision and page is by page ID.
Bug: T297431
Change-Id: Icc1df0c9ca5345e65ef5f8daf0815013d7db0943
The 'legacyPrimary' links will take you to the section
the comment is in and should be used when you don't have
access to comment IDs.
Bug: T296018
Change-Id: I944feb90e7c3a69f81366f42fa110c58cac26dbb
* Add a N/A value for edit counts from anonymous users
* Only oversample with $wgDTSchemaEditAttemptStepOversample if the edit
is from DiscussionTools
* Consider $wgWMESchemaEditAttemptStepOversample for oversampling
Bug: T286076
Bug: T295995
Depends-On: Ieb3f6c6e1775c1ef53747c37003b17e3634d1c44
Change-Id: I91245a61dfbde8b5ec9b2893b9170cc4d73f7b0a
If the user talk edit or mention coincides with exactly one new comment:
* Change the primary link to be a direct link to the comment
* Add a text snippet to notifications that don't already include one
(user talk edits that are not new sections).
This is done for all such notifications, regardless of whether anyone
has topic subscriptions enabled.
Bug: T281590
Bug: T253082
Change-Id: I98fbca8e57845cd7c82ad533c393db953e4e5643
For automatic topic subscriptions, I plan to introduce a third
subscription state to indicate them. This patch includes minor tweaks
I wanted to add while working on that:
* Introduce constants instead of numbers
* Remove a TODO that doesn't seem like a good idea any more
* Remove a `"length": 1` on sub_state that did not do anything
(but it might have been meant to indicate that it was supposed
to be a boolean, which would be wrong)
Bug: T284836
Change-Id: I6e6096968ad38510102287bccd349090b6ca4280
Notifications are bundled by section, so instead of linking to the
comment, link to the section.
Additionally, add a parameter to the URL listing all the comment IDs
from the bundle, and highlight them all and scroll to the topmost one.
Having to handle both URL fragments and URL query parameters makes
this code kind of a mess :(
Also, some unexpected changes became necessary:
* EventDispatcher.php: Store the section title in events using
HeadingItem::getLinkableTitle() instead of ThreadItem::getText().
The result is mostly the same, except in case of wacky markup like
images or extension tags. We can more reliably use it to link to the
section on the page, and we already use getLinkableTitle() when
generating edit summaries in the reply tool for this reason.
* dt.init.less: Change the mix-blend-mode for the highlights from
'multiply' to 'darken', so that multiple overlapping highlights do
not look more opaque. This affects how the highlights look on
non-white backgrounds and images (they're less blue, and on darker
backgrounds entirely invisible), but it seems worth it.
Bug: T286620
Change-Id: I21bb5c003abc2747f0350d3f3af558dfb55693e9
The code (prior to d25825a754) assumed
that level 3+ headings would always follow a level 2 heading or the
placeholder heading, but we don't generate a placeholder heading if
there are no comments in section zero.
Add more tests to confirm that comments under level 3+ headings (that
are not sub-headings of level 2), and level 1 headings, are ignored
when generating notifications, and do not mess with normal headings.
Bug: T288775
Change-Id: Ic57b56752a4797cb01234f66e0ed7b849752bd70
Use `DOMCompat::getBody( ... )` as a nicer getter than
`->getElementsByTagName( 'body' )->item( 0 )`.
Remove overly defensive checks and redundant annotations on its
return value. Since we're dealing with HTML documents throughout,
the document body is guaranteed to exist.
We previously needed some of them to convince Phan when it thought
the body may be null, but this seems to no longer be needed.
Change-Id: If7aee7b6adbfa78269c7ba28b26a6eaa21fe935b
In case 4 and case 6, no notifications are expected. In all other
cases we now get the expected notifications.
Bug: T285528
Change-Id: I9e813bb3a053bc1232783f9eae1ad75672b4fa7e
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
The user interface only allows you to subscribe to level 2 headings.
But we would generate events for whatever heading was the closest,
If it was e.g. level 3, no one would receive that notification.
Now we generate events for the closest level 2 heading, or we don't
generate the event at all if there isn't one (if the only headings are
of level 3 and below, or level 1, or if the comment is added before
the first heading on the page).
Bug: T286736
Change-Id: Iae99853070e353ab81c9cc29ef1d53c877adfc66
We used an internal API requests to fetch page content because it was
easy, but there's no way to guarantee that it returns data from the
primary database.
Use ParserOutputAccess::getParserOutput() to fetch from cache if
available. Also, use canonical output instead of user-specific,
not that it should matter.
Bug: T285895
Change-Id: I7dcd9659be77746dc2a0c4eeae2319887936b555
…without making the topic subscriptions feature available in user preferences.
Follow-up to these commits, which added these checks in ad-hoc ways:
* 9420f22e9d
* f3422f40a6
* 23a490deca
* a555db7892
Bug: T284491
Change-Id: If2e3fb1e06d1cc489fbca14796ed77c83bb52991
If the revision from which we generated the notification has been
deleted, we shouldn't include the content snippet, nor the direct link
to the comment (because the fragment ID is generated from the content).
This matches how Echo handles mention notifications.
Change-Id: Ica939f3a4efd39d0c295511d58280d3f9d584129
