Babel: it-N, en-3, fr-1, de-1
Mon, Jan 24
I'm not sure what's going on -- for starters, there haven't been changes to AF in REL1_36 that crossed that code path, nor to core's HTMLForm.php file. However, this code was almost certainly working when 1.36 was cut. Second, the assertion before the first failing one ensures that the current page is Special:AbuseFilter/new, and this is confirmed by the image linked above. The view picker code in 1.36 is essentially the same as today, and if the subpage is "new" it instantiates an AbuseFilterViewEdit object, not AbuseFilterViewImport, which is in the above stacktrace. The code in question is also pretty simple, and I don't see how it could confuse "new" and "import". As such, the only possible explanation is that the special page is being instantiated with the wrong subpage (by SpecialPageFactory). My guess would be something messing up with Titles and leaking some state. I seem to recall a similar issue in the past (for unrelated code, I believe), where the Title wasn't correct after form submission.
According to https://integration.wikimedia.org/ci/job/quibble-composer-mysql-php73-selenium-docker/13195/artifact/log/bad-data-results-in-an-error.png, there's an actual problem with the code...
Sun, Jan 23
FTR, this seems to be still happening. Current backtrace:
Thu, Jan 20
Mon, Jan 17
I've regenerated the stubs using make_stub, but the final privates are still there. At this point, I assume it's something within the extension that sets both modifiers.
Tue, Jan 11
Based on my local testing, my guess was right. This was caused by r747596 -- notice how $slotRole is not passed to onEditFilterMergedContent. OTOH, it's really not that patch's fault, since the parameter is not officially documented in the hook documentation, nor is it present in the EditFilterMergedContentHook interface (see T288885).
Can this be closed now?
The first thing that comes to mind is that whoever is firing the EditFilterMergedContent hook is passing SLOT_MAIN instead of the actual slot being edited. I'll try to reproduce this locally and see if that's indeed the reason.
Wed, Jan 5
Tue, Jan 4
Dec 23 2021
AIUI, the actual bug here is the page going gray instead of displaying a warning message. The warning only appearing once is how the warnings are supposed to work.
Dec 21 2021
This is a bit problematic... AF itself does not have access to the global request when it fires the hook, and accessing the WebRequest in general is not easy (unless you have a ContextSource). Also, I'm not sure what the future of WebRequest is, and whether we should treat it as a value object or as a service (currently it's something hybrid). If it's the former, then MW core hooks should pass it; if the second, then it should not be passed by the hook, but injected into the hook handler.
I'm not sure if it's doable... The main potential issues I can think of are:
- It might need A LOT of time and memory
- We need to make sure that vendor dirs are only parsed once
- Different repos have different configs
- More code to analyze means that phan will end up analyzing some methods in a different order, which is going to have a noticeable effect on analysis result
Seems to be fixed now, thanks!
Dec 20 2021
I don't know precisely how that job works, but it seems to be using the composer-php72 image, on which we do NOT install our version of php-ast, so I guess it's just using some old version.
Dec 17 2021
Before adding the extension to translatewiki, I'd also like to create an alias file with the translations of the special page name (I don't know if things will work with an empty $specialPageAliases).
Dec 16 2021
Dec 14 2021
Dec 10 2021
Dec 9 2021
Polymorphism doesn't work well with static analysis. Strings can be callable, so phan doesn't know whether the provided string is meant to be used as a callable, or as something else (in your example, a method name). The closest solution would be to disable the validity check for callables if the parameter can be callable|string (and probably other combinations, e.g. callable|array), although that would have false negatives. Here's the upstream issue for that: https://github.com/phan/phan/issues/1648
Dec 8 2021
I think it would be better to generate them ourselves (instructions); PHPStorm stubs may have limited compatibility with phan IIRC.
Dec 6 2021
Dec 3 2021
So, we think that changing the code to use $elOrLayout.fieldWidget.wasDisabled should fix this issue. The reason is that this property is set and updated in setDeleted(), which means it keeps track of state changes, whereas the wasDeleted property set in the hide-if logic is never updated. I think this change is correct, since the two properties should in theory always have the same value, but would like to confirm that. @matmarex I see you wrote this code a few years ago. Do you by any chance remember if there was a particular reason not to use the wasDisabled property of the inner widget?
Dec 2 2021
I'm currently a full-time employee.
Dec 1 2021
Nov 30 2021
No new features were added, so we would like to make sure that Special:Undelete and the undelete API module are still working as expected; a few suggestions:
- Ensure consistency when undeleting files (as opposed to articles)
- Test behaviour when selectively restoring only some file versions and/or page revisions
- No need to test this on pages with many deleted revisions; in fact, that will probably timeout and fail
- Ensure that errors are displayed correctly, for instance if there are no revisions to undelete (e.g. submit the form twice)
Nov 29 2021
@Dzahn On second thought, I'd rather create a separate WMF account, since I didn't realize that my wikimedia.org email would also be used for gerrit and the cloud. I need to create a new WMF account on Wikitech, right?
Nov 25 2021
As I said, if my wikimedia email needs to be in the puppet file, that's fine. I do prefer not to use my real name publicly, but I believe this particular instance to be acceptable (as in, not too public).
Nov 24 2021
This means that Phan can be used in Docker, which is extremely annoying to not have, as every change has to be round-tripped though Gerrit.
Nov 23 2021
Not as much as before. As for the email, it can be changed if necessary; otherwise, I'd rather leave my volunteer one.
Ready for QA now! As a reminder, no new feature was added, so we're still in the "ensure nothing broke" phase. The suggestions at T288758#7404009 are still valid.
Nov 22 2021
Nov 21 2021
Nov 20 2021
Nov 18 2021
Nov 15 2021
Actually, I believe it's much better to wait for r730775 as well. The test suggestions above are still valid, I'll move this back to QA when that patch is merged.
Nov 12 2021
Nov 10 2021
This is actually still relevant. In particular, given T293851, it would be great to have pcov in wm apt, assuming that we're going to switch coverage jobs to PHP 7.4 at some point.
Thanks for reporting this. Looking at the changes to DeleteAction, this was actually caused by r711477: FileDeleteAction has a check for redirect files (and also non-local and non-existing files) which triggers a normal article deletion. This is currently happening in FileDeleteAction::tempDelete by calling parent::tempDelete, but that's not enough: everything is still happening inside the FileDeleteAction instance, hence any call to a local method (i.e. $this->foo()`) will use the method in FileDeleteAction (and not DeleteAction) if it exists. r712747 does make it worse by showing the message, and r713299 does fix it by reducing inherited methods, but the underlying problem still exists and is waiting for an innocent code change to trigger similar bugs again. So I'm going to do the switch in WikiFilePage instead.
Nov 9 2021
Sam and I discussed this today, here is a summary.
And now I set English again and I can reproduce the bug... Nevermind. I've verified that the quick fix above is enough for the immediate problem, so I'm going to merge it. The core change with disable-if is still something that would be nice to have, but for now I want to fix the issue ASAP to try and understand how all these bug reports about GP are related.
I changed the content and interface languages to italian, and now it works. The "gender" field is no longer disabled. WTF?!
I can reproduce this in production, but not locally. If I set a global preference and go to Special:Preferences to set a local exception, the preference field is greyed out as expected. @Samwilson any idea about that?
Nov 8 2021
We agreed that these browser-specific bugs won't be addressed in this phase. The CSS+JS solution provides basic support for copying from diffs, which is what we wanted to implement. Better support will come with T270775.