ApiDiscussionToolsPageInfo and ApiDiscussionToolsCompare in direct parsoid
or VRS modes tries to fetch HTML using VisualEditor thus stashing the
HTML gotten which we don't want, we only need it for viewing in these cases.
This seems like something that was/is already happening in RESTBase. So for
APIs in DiscussionTools that need the HTML for viewing, just get it from
parser cache and not stash it.
Bug: T323357
Change-Id: I101c1e84739a2ac1f562f2f7bdc4b8f53d9f3b23
This is a better way than a loop in the test code.
When using DISCUSSIONTOOLS_OVERWRITE_TESTS, if both desktop and mobile
cases fail, both will be updated now. Previously the first failed
assertion would stop the test after only updating one file.
Change-Id: I4ce6f45b047e02c9f00024a9e5057adcb0e28047
Just to confirm that this works correctly, because I broke it a few
times in different ways while working on T321121.
In this scenario, discussiontools_item_revisions contains rows for
the oldest and newest revisions of pages containing each comment
distinguishable by name, as well as extra rows for the newest
revisions of comments indistinguishable by name, so that they can
be looked up by ID.
Change-Id: Ic8450a6b082ed343dd633d3a43c50696b5d6d2bb
discussiontools_item_pages row itp_id=5 has incorrect
itp_oldest_revision_id. It should point to the older of the two
revisions with the same time, but it points to the newer one.
discussiontools_item_revisions row is missing.
Depends-On: I56f0e161e5438d5f77b7d53d4db7411f90f97d05
Change-Id: I61ed42515891a84729455a7a32c98276c7cacd40
We originally used 'templates' because it seemed like an obvious
choice for HTML files, and because 'packageFiles' requires extra code
to include anything that isn't a .js or .json file.
However, the templates are expected to be HTML fragments rather than
whole documents, and they are parsed in a particular way that takes a
lot of code to clean up (which we needed to do, because we use the
same test files for testing PHP code).
I tried doing it in the 'packageFiles' way, and the extra code doesn't
seem that bad in comparison after all. Moreover, the 'templates'
mechanism (when used the intended way) feels vaguely deprecated in
favor of Vue.js, and I'd rather move away from it.
This makes the tests faster too (probably mostly thanks to the removal
of the clean up code) – on my machine they go from 1800ms to 1500ms.
(Simplify linearWalk tests, as we no longer need to do weird things
with document fragments to get consistent outputs in PHP and JS.)
Change-Id: I39f9b994ce5636d70fea2e935a7c87c7d56dcb26
Many methods are covered indirectly, and using method-level
@covers filters means these are reported as not being covered.
Change-Id: I94eb3e8c48209ff0b6bfc09e18c93555bb167e8f
The implementation in Parser doesn't descend into sub-thread.
Re-use the getThreadSummary method in ThreadItem and traverse
the thread properly.
Bug: T298617
Change-Id: I318d9012eb83f37ccbe463923524ef2e9f995ced
In the future the notifications can be improved to look up
the new location of the comment, using the permalinks data.
Depends-On: Ia8a21749a8edc20f34b2a3e445278ea6922b9109
Bug: T299657
Change-Id: I5f5e7b73fb84ff0d31fb8260b24066a17da71628
We can no longer change IDs so easily, because they're stored in the
permalink database, so remove this mechanism to make sure it's not
accidentally used in the future.
Change-Id: I392ee1f49c48fc2f23d05e9a37c643438b4f2b9a
* clock, userAvatar, speechBubble were dropped before topic
containers was merged
* Load ellipsis & edit icons on mobile only
* Load bell & bellOutline for topic subscriptions button
Change-Id: I77d1336627b564be756e3ec50b4686b8f9ac72dc
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
We were calling Title::newFromText() before setupEnv(), which meant
that the title for each test case was parsed using the default rules
for English, rather than the rules for the specified wiki.
This only makes a practical difference for tests with self-links.
Changed the only such test to demonstrate the fix.
Change-Id: I45561f1c9f0d149e2b743f0000b742bf6fc014af
Since times immemorial, and for reasons lost to history, our test code
was adding an extra <div> wrapper before parsing the HTML used for
tests. This wasn't a problem, until now, because I want to add some
tests for T303396 that need to check that the *real* wrappers present
in some test cases are handled correctly.
Changes to test cases mostly remove a leading "0/" from serialized
ranges, corresponding to removing the extra wrapper.
Change-Id: Ia50e3590538c8cd274b02d2a937ba1a3fbb4ac89
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
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
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
The Data class contained utilities for two unrelated purposes.
Split each half to a separate class.
Notably, this improves the signature of the getLocalData() function.
Change-Id: Icde615fb9d483fee1f352c34909b37f8ffde8081
(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
