We no longer need to support toggling visual enhancements without
reloading the page, so we can remove the extraneous set of buttons
to reduce HTML size.
Bug: T322457
Change-Id: I54e57c754c54b7e611069f9832d1ebabf141a396
In this case, the generated regexp would match the '/local' part in
the generated URL. Prefixing 'https://local' is no longer necessary
since 10899af666.
Add tests for this, and some tests to cover T261711 as well.
Bug: T358321
Change-Id: Idf54deba13f30b799b7b8d17de1897bc90f95701
We never a DatabaseThreadItem back into the database. If someone
wants to do this in the future they should be thinking very
hard about it first.
Change-Id: Ie073c030905e31d81cb75bc9c46d5bb7c5d85b04
This appears to reference the retry that happens due to
REPEATABLE_READ, but that seems like an implementation
detail so not really useful to have in in the function
name, nor does it disambiguate it from another function.
Change-Id: Iacc95b74f25c166545734818efb1b30e18a1754e
This was intended as a temporary config to facilitate a
staged rollout (T331635) which has now completed.
Change-Id: I432ec0a24b8e8c12b62556ff6703abff32a2fced
We already supported plain headings without the 'mw-headline'
wrappers, but now we need to parse the 'id' from a different
attribute.
Needed-By: If04d72f427ec3c3730e757cbb3ade8840c09f7d3
Bug: T357723
Change-Id: If85f89c40834618f23dc0ace2e599efb3b6d5ed4
In certain cases the parser could go back rather than forward after
finding a signature, causing it to find the same signature forever
until it ran out of memory.
Test cases coming later in a separate patch.
Bug: T356884
Change-Id: I8ac72b05e5e4ed45e6119c012a69708c9d8eda29
* Keep the information about this in the API result, which we were
previously omitting
* Use it instead of duplicating the logic client-side
Bug: T356131
Change-Id: I7e7342e9d94a171b5ef56e646871c18c8c39c44c
In production, indexing the full history of every page will take
too long, so we can't rely on being able to find the target heading
text in the history of the current page.
When this search fails, fall back to the following searches:
* The heading appears in the current revision of a sub-page
* The heading appears anywhere on the wiki, but only once
Bug: T356276
Change-Id: I90e92cb9c85aaf6fb2355f842450981bbe6abf2d
This will be present in parser cache output and can
sometimes be mistaken for user page links.
Bug: T356142
Change-Id: I800b23d8466f72affcadfa336aab07abf7f8d79e
This reuses the existing comment parser code in HookUtils and makes the
DataUpdatesHooks and EventDispatcher code consistent.
Bug: T354792
Change-Id: I58a71b26b3f47898a68a29098a8105ee844403dd
This reverts commit 1cf6000d18.
Reason for revert: Want to do another round of visual diff testing
on wikitech as an anonymous user. Will revert this after test is done.
Change-Id: Ic3facb7ecd9c932be348d47d6a38cff71cd7e1db
To get ready for the subtopic subscriptions feature, this patch
separates the code that adds the topic container from the code
that adds the subscribe button.
The idea is that in the future, an H3 would need a subscribe
button, but not a topic container. Whereas an H2 would need both.
Bug: T275943
Change-Id: I6a82773ed200dcb4083ac38a4d41838f639c097e
This will let us render reply links on wikitech and run visual diffs
(which runs in anonymous user mode). This will be reverted after
the visual diff test run.
Change-Id: Ibf175a7f5b1e68f66c257fc26ba9e4b55f752fbd
This data was added to core in I328f533e6cdb11c0c3a873d23bab1a113dfa39be
and it will have been in production for 4 weeks next week which is
enough for all content to have rolled over.
Change-Id: I3d568eed56446f26aa329bfa554d609b8bcb973a
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
* Looks for heading IDs matching "h-<heading text>-%" that once
existed on the target page.
* For such IDs, finds where those items currently exist,
presumably in an archive.
Pros:
* Doesn't need to know anything about the local wiki's archiving
conventions, so can be deployed universally.
Cons:
* ID conflicts will return matches in unrelated archives, e.g.
MassMessages.
Bug: T349653
Change-Id: Ie94efd0503e9f4689d3421babe445f9f4e2b4fb7
User-options related classes are being moved to the MediaWiki\User\Options namespace in MediaWiki Core; reflect that change here.
Bug: T352284
Depends-On: I9822eb1553870b876d0b8a927e4e86c27d83bd52
Change-Id: Iaf161106c323461929abe9b8a021bbb3e34c4ae7
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
This reverts commit 7aaaf51dfd.
Reason for revert: This is not right and doesn't work either.
See T351461#9358034 for why this strategy will not work right now.
We need a different strategy to prevent duplicate transforms if
they continue.
Change-Id: I97efee9197359ecdccdf89a0be850a707a11cc98
* getText() could be called multiple times on a ParserCache object
which would fire the ParserOutputPostCacheTransform handler
multiple times.
But, I could not track down how this could happen right now.
* As a separate issue, while conceptually there are no restrictions
against calling getText() multiple times, there is a semantics and
performance issue if that did actually happen. getText() does a
bunch of transformations and makes no effort to avoid duplicate
work. It will accumulate more transformations over time via the
OutputTranform pipeline and it is preferable for getText() and/or
the OutputTransform pipeline to guarantee semantics where the
pipeline won't be run multiple times on the same content. That will
free both hook handlers (like this) and the transforms themselves
to avoid checks as in this patch.
This patch should be reverted once such a change is made to core.
Bug: T351461
Change-Id: If5dfa0954e3fd2b7dbea1ed29b475be07f0f3986
* This patch enables DT to work with Parsoid HTML without changing
the functionality for legacy HTML.
* The code comments document some of the decisions being made here.
Some of these decisions are temporary and need better solutions
but this patch will let us run visual diff tests and expose any
other latent bugs.
TODO
----
* We need to add new tests to verify CommentFormatter expectations
for Parsoid HTML. I'll tackle this in a followup patch.
Known issues:
-------------
* Performance: Since the getText() transformed output in ParserOutput
is not cached, if DiscussionTools is to switch over to Parsoid HTML,
we have to add some form of caching of the transformed output because
transformHtml can take a couple seconds in the p99 case which is too
long to render uncached!
* Longer-term: Since this hook is called when getText() is
called, all calls to getText() will now invoke this handler
(which will return but still has to do a bunch of checks to
determine this won't apply). Presumably, transformHtml() is
idempotent because when some other code (other extensions, for ex)
calls getText(), we will run the transfromHtml() on previously
transformed content.
My understanding is that getText() is going the way of the dodo
and that getText() callers will have to explicit call the output
transform pipeline code (and presumably this issue of repeatedly
calling the same transforms on previously transformed content will
be addressed there).
* Some CSS doesn't apply to Parsoid HTML because intervening <section>
tags interfere with existing query selectors -- will be addressed
separately.
Bug: T341010
Change-Id: I9846193656cdc658f5237df0a133d9d4dcc20d00
According to EventLogging 'editattemptstep' data the error hasn't
happened in the last 90 days, and according to Logstash data the
warning hasn't happened in the last 90 days either.
The problem this guards against shouldn't be possible in a world
without RESTBase, but keep the checks as assertions just in case.
Change-Id: Id7eaf14294f8a7bb877f6a0e00a90976e560fc54
While this method is not a huge bottleneck in this codebase it still
sticks out because it calls end() and array_pop() literally millions
of times. (Tested by running the unit test suite, which currently
takes about 30 seconds on my machine.)
Because of the way the method is used in this codebase (see especially
ImmutableRange::computePosition) $a is almost always a sub-element of
$b, or the other way around. It's almost never necessary to go all the
way up to the root element. We can use this additional knowledge and
stop much, much earlier. The extra code is worth it because we know it
will succeed very, very often.
When I measure the runtime of this method alone it goes down to less
than 1% of the previous runtime. The final loop at the end of the
method is almost never executed now (about 30 times in 15,000).
I also micro-benchmarked the final loop and optimized it to work with
passive array-indexes instead of actively manipulating the array with
array_pop().
Change-Id: Iffcaa8848780a85fde38e322649050c687567f29
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
I was curious why running some of the PHPUnit tests in this code base
takes so long. While I could not spot an obvious bottleneck I found
a lot of code that is extremely hot, e.g. called a hundred thousand
times. A few obvious optimizations are possible in this code, e.g.
not calling the surprisingly expensive DOMCompat::getClassList
multiple times.
Change-Id: If22bbc1aedd2c36db1ab2343de5737009050b7bb
There is nothing in this preg_replace call that needs to be executed
"as code". A normal preg_replace can do the same.
The pattern looks a bit different but really does have the exact same
effect as before.
Change-Id: I3597d632f2ecbe5b7ccef39a394075327c9bea79
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
Why:
- Per T338534, we want to display the overflow menu next to topics and
comments. Supporting topic-level placement is a little more
complicated, so just go with comments for now.
What:
- Rework the is-mobile check to allow the code to run on desktop/mobile,
while excluding topic-level replacement on desktop (T342627)
Bug: T338534
Bug: T342625
Change-Id: I520c377120e16aa3a6fedcc8c39075958a942e4c
Why:
- The button should only display for Minerva
What:
- Consult the skin name before adding the "Edit" button to the menu
Bug: T342251
Change-Id: I52cf2ca0663a4de0ee7add82910e745bcabf1c5f
'editable' refers to the section being editable, and is only for
use by heading items, not individual comments.
Change-Id: I3fb5841609c40a928071cea8987cf035ade464c2
Since 92f5cfd8 we support "mw-notalk" to suppressing comment detection
in pages or sections.
Until now, it only worked when the comment timestamp was surrounded by
a marked element. However, when a marked element was directly adjacent
to a comment, it would sometimes become a part of the comment range.
This can no longer happen now.
Existing use cases for this were the {{outdent}} and {{tracked}}
templates, which we handle specially since 50ad5bb2 and ddd391b6.
It's a bit ugly to hardcode specific templates like that, and this
provides a better solution for the future. The added test case
displays some other potential uses.
Bug: T324132
Change-Id: I7ffd299ef5957b35da8d01f9a0ed5a7a9a78be83
Only needed for phan, under php8.1 this internal
function is deprecated about null.
The property $subscriptionName is not null when $subscriptionTitle is
not null and current code is safe, but phan cannot see the dependency
between both properties
Change-Id: I9e67309ec25a70c5de91d7f3e8f18447f205a4e4
Why:
- We want to provide contextual actions adjacent to comments, not just
topic headings.
What:
- Consolidate the logic for embedding the overflow menu button HTML into
a function, and add this for both topic and comment items.
- For this patch, this is done only for mobile; in a follow-up patch, we
can add support for desktop
- This doesn't use a feature flag as 1) no one is implementing the
DiscussionTools hook yet, 2) the 'edit' menu item is disabled at the
comment level. Merging this change would be a no-op until an extension
implements the DiscussionToolsAddOverflowMenuItemsHook.
Bug: T342251
Change-Id: I15a151f151e2fa04398876b559d93d45c42f6ef6
Why:
- We want to allow extensions to register interactive menu items in the
overflow menu.
What:
- Create a PHP hook to allow extensions to provide menu items
for rendering in the overflow menu
- The hook allows for registering resource loader modules required by
the menu item
- The hook passes in some contextual information, like the thread
item data, context source object, and if the page is editable
- Create a JS hook that fires when a user selects one of the menu items
- Example implementation: Ie9afbedb4f24cbd75eb48bb21dc9f6d8d732d853
Misc:
- Remove b/c code that existed to handle a transitional period where
JSON encoded overflow menu data did not necessarily exist in the
parser cache
- Rename code instances of ellipsis button / data / menu to refer to
"overflow menu"
- Some renames will have to wait until parser cache is updated; these
are noted with TODOs
Bug: T342251
Change-Id: I5f2a51791f8ba7619d1399a4b93111e9bb44e172
In T339882 we thought that these rows are harmless, but for the
specific case of itr_items_id that's not true.
Bug: T343859
Change-Id: Iebfa583d7d4b00a59291f91ec84c41241d5469d4
For some reason this apostrophe is treated like the start of a string
by Gerrit's syntax highlighting, messing up the whole file. It annoys
me very much.
Change-Id: Ie1ce88eefda6ae2a8a73237556affba4cbc6db4b
This approach is borrowed from PageQueryPage in MediaWiki core,
and used in a few other places as well.
Bug: T345648
Change-Id: I7fbc3c3c1133da78eb9f15de9b2a51a90bcd1980
Check whether the page given to DiscussionToolsDebug exists or not.
Without the check, it throws Wikimedia\Assert\PreconditionException.
Add 'exists' => true in the getFormFields() array.
Bug: T338480
Change-Id: I57222f60cb5c5645e6bc97b86be22c502c85d650
When rendering a preview of the comment in order to check whether it's
signed, use the previously acquired temporary user username for the
signature.
Depends-On: Iec8a15dadd595bed0f7e54f907fbb8e192b45cf3
Bug: T331397
Change-Id: I7aeb1cc4c107ed752dc805405780a7609a6d4d3c
What:
Growth wants to use ApiDiscussionToolsEdit to
post questions on behalf of new users (to make use
of the auto-subscribing logic DiscussionTools has).
This seems like a good moment to make the API
non-internal.
What:
* Add support for 'tags' parameter
(this doesn't actually work w/o the depends-on)
* Add autosubscribe parameter to give more control
to callers over whether autosubscription is used.
* Remove isInternal()
Bug: T343339
Bug: T333632
Depends-On: I0ac60ca8473fe28461b2da60f9911baac4994388
Change-Id: I39727de40557d2494f4d60bf224490caaedfdee1
Parsoid can be expected to throw limit exceeded exceptions so a more
graceful response should be provided.
Bug: T325298
Change-Id: I6bedc7639a68311d5247331d5e53f88c004ebc5e
For some unknown reason our updates are being processed more than once
at the same time. Ignore or work around the "duplicate entry" errors
caused by that.
Bug: T323080
Bug: T341811
Change-Id: Iaae1dc2d5ed5bf4ac6760fb1d39dc21f2af89e9a
This reverts 08e1073f58 (T322701),
which does not work, and instead uses the same solution as
4439e934429617cc77f34cc9c783f0b52a06c920 (T247553).
Bug: T339882
Change-Id: I1b6772b1cdfd0d7e1c603333a2e43152d3591c38
