Reasons:
* Various other methods dealing with ranges already live there
* It would be neat if ContentThreadItem was just a value class
without a lot of logic, similar to DatabaseThreadItem,
particularly for writing unit tests
* The methods access global state through Title, which can't
be fixed while they're in ContentThreadItem (see I9dfccc83)
The computation is now always done, instead of only when needed,
but that's a small drawback, since it's fast (fast enough that
I don't see the difference in the time taken when running tests),
and we were already computing it for all comments in many places.
Change-Id: Ic718a964e309ae3a8e15e299081f46d4db860731
This might be a matter of personal preference. Not sure if it's
worth it. Both is well readable. On the other hand, the method
exists. Why not use it?
Change-Id: Id66fc6c888db6ae1cf28e60a51f90d9ae2cdb6ee
It does the same as before.
I think performance is not a concern here, and wasn't my motivation
either. But I hope this makes the code easier to read and to reason
with.
I added a pure unit test case (without involving an actual Language
object) to cover the previously uncovered digits feature.
Change-Id: I6a0fc86035817eabb42b55e58183ae094c052aa6
I was curious why running the CommentParserTest takes so long. I
found this is one of the bottlenecks because it's called so often,
but many link titles that are parsed as user names turn out to be
something else. This little hack speeds up the test by 15% and has
probably a similar impact in production scenarios.
Change-Id: I5a0b3a49ba5793c8a345baaa7118fed500c082b6
MediaWiki's PHPCS plugin requires documentation comments on all
properties, unless those properties are typed.
This has potential to introduce bugs – in particular, because typed
properties without a default value will throw an exception if their
value is accessed before it's defined, while previously they defaulted
to null. I fixed this when I found it (making them nullable and null
by default), but I may have missed some cases.
Change-Id: If5b1f4d542ce3e1b69327ee4283f7c3e133a62a0
https://wikipesija.org is currently using ISO 8601 as the default date
format. The format is xnY-xnm-xnd"T"xnH:xni:xns and 'xn', 'm', and 's'
need support added.
Change-Id: I235098a578eb92ddd23ea47fa23d60df4b28f590
Also merge setMwGlobals() calls because they are really expensive.
Also utilize the more readable str_contains() and related.
Change-Id: Iebde6aa17c2e366f0c0a98fe13a454f6a06c299b
This can be done within sections using CSS:
* mw-notalk
Or at a page level using a magic word:
* __NOTALK__
"notalk" suppresses all comment detection, treating the content as
not containing any comments even if there are signatures present.
Bug: T295553
Bug: T249293
Change-Id: Ic1d7294bafcf7071e16838e70684ecadd7bc6fd3
Change code to match the documented consensus formed on T321683:
https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP#Exception_handling
* Do not directly throw Exception, Error or MWException
* Document checked exceptions with @throws
* Do not document unchecked exceptions
For this extension, I think it makes sense to consider DOMException an
unchecked exception too (in addition to the usual LogicException and
RuntimeException).
Depends-On: Id07e301c3f20afa135e5469ee234a27354485652
Depends-On: I869af06896b9757af18488b916211c5a41a8c563
Depends-On: I42d9b7465d1406a22ef1b3f6d8de426c60c90e2c
Change-Id: Ic9d9efd031a87fa5a93143f714f0adb20f0dd956
* Detect comment separators at the end of comments too
* Consider TemplateStyles associated with ignored templates
This unexpectedly improves a lot of cases other than T313097 too,
mostly where <br> or {{outdent}} was used within a paragraph:
splitting comments that were previously jumbled together, or restoring
content that was previously ignored for apps / notifications.
Bug: T313097
Change-Id: I9b2ef6b760f2ffd97141ad7000f70919aeab7803
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
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
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
Change the HeadingItem constructor to take a 'null' headingLevel
and store this internally with the constant. Change the JSON
serializer to convert this back to null.
Change-Id: I27508eed75d94b99c5189548919309f8da7deb75
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 previously ignored them before timezone indicator (e9c401e3aa),
but they can end up in other places too, e.g. after the time.
Now we ignore them after every token. This is way overkill, but it
shouldn't hurt.
Bug: T308448
Change-Id: I20f7aaa34dba23f2a2faf1be258c1aea32ab770f
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
We call CommentUtils::getHeadlineNodeAndOffset() before constructing
the HeadingItem in CommentParser, so the range's startContainer
is always the headline node.
Change-Id: I2afb6ba9100e785cd91f31d82f4cea59fa8b5443
`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
Also, in ThreadItem::getSinglePageTransclusionTitle(), we don't need
this terribly complicated method.
Change-Id: If02c09aaa2f4dd66b2bc253a1edec4ea107564ee
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
It's an arbitrary limit, it seems harmless to relax it to support the
use case in the task, even if it's weird.
Bug: T300949
Change-Id: I7c895c7019726758bbae3183b9c3ecbd9eabcf38
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
