May 2 2022
May 1 2022
Apr 28 2022
I think this is done in core, at least for now. I'm sure more instances will crop up for a while to come, so it probably deserves checking again sometime in the future.
Apr 27 2022
Command that should find usages:
Apr 26 2022
So a basic question: if I have the ID of a non-local wiki, how do I find out the corresponding database domain? This is necessary to implement UserRightsProxy::invalidateCache as a method of UserrightsPage.
Apr 24 2022
Apr 14 2022
It changes the user table for the other wiki, right? Surely that's why UserRightsProxy::invalidateCache exists to begin with?
This was part two, but I forgot to add the "Bug" annotation:
Apr 13 2022
What should be used for invalidateCache? Should UserRightsManager have something that can do that, or should it go elsewhere?
In case it's relevant, I get a whole list of those "premature access to service container" warnings also, with no extensions installed. The only change I made to the LocalSettings.php generated by the installer is loading the Vector skin.
Apr 12 2022
I didn't find any more cases where this is triggered, so I guess let's consider this resolved for now.
Apr 11 2022
What's the procedure for backporting? I didn't find it in a quick search.
ArrayAccess and JsonSerializable are two culprits that I find when running unit tests. Interestingly, the change doesn't seem to affect Iterator or Countable.
Reopening to deal with the remaining issues of this nature. (I figured that made more sense than opening a new task, right?)
Apr 10 2022
Why not just make it a fixed name based on the name of the setting? Then we wouldn't even have to array_filter(), we could just iterate through the methods with reflection.
What's the problem with a one-pass iteration to automatically collect all the settings with dynamic defaults? If it's really measured to be a performance problem, it could be done very easily with code generation to avoid repeating the work on each page load.
This should only be set if Title::loadFromRow is passed a row that has page_restrictions set. But, sure, cache eviction sounds like a reasonable idea here.
Apr 7 2022
It turns out I had a phpunit.xml from ages ago that I didn't remember existed (nor did I remember that such a thing existed). Now everything works fine.
Oh, and the same happens for any other files I try, so it's not just a problem with this file (which I'm in the process of writing now).
I have no idea where I originally got my vendor, I installed it years ago. I probably just followed whatever the instructions were on the wiki.
Apr 6 2022
The following are set by default to the value of another setting:
So it occurred to me -- what's the purpose of having a separate class to compute dynamic defaults? Wouldn't it make more sense to put this information in includes/MainConfigSchema.php by allowing 'default' to be a callable that will be executed and return the default value based on the other values that have so far been configured? Of course you'd have to make sure to call the callbacks in the right order, but it seems like a much better solution than pushing this logic all off to a separate class. Dynamic defaults would then be set when SettingsBuilder::finalize() was called, or something like that.
Apr 5 2022
Why does the hook not work? Why can't you register a hook for CanonicalNamespaces like
It sounds to me like his problem is that he wants to set config globals after services are initialized and have the changes be picked up by services. This isn't caught by any existing warning, and can't really be, unless we have some way of making the config globals read-only when services are initialized.
Apr 4 2022
This looks like a bug in the extension. Extensions can either declare namespaces statically, in which case they should work fine; or use the CanonicalNamespaces hook, in which case it should work as long as the hook returns consistent results (and of course is registered before service initialization). Changing configuration globals after service initialization is not supported and will not be supported.
Apr 3 2022
Yes, RestrictionStore doesn't manage its cache and it could in theory grow boundlessly. This is true for all the cache entries there -- I'd think oldRestrictions should be the least of the problem, since the vast majority of titles with restrictions shouldn't have an entry in it. Is this causing real-world problems? Do we have a nice off-the-shelf way to limit the size of caches like this, or best to just write a little method and run it every time a cache entry is added?
TestAllServiceOptionsUsed probably should do nothing if the whole suite is not being run?
Aug 9 2021
Aug 1 2021
Jul 28 2021
Apr 22 2020
Oct 30 2019
Oct 29 2019
What type of errors? Keeping in mind, again, that it only affects type hints on functions, not anything else.
Also, as we start using scalar type hints, we should require declare( strict_types = 1 ) for all files. This will not affect backward compatibility, because it only affects calls to functions with strict type hints. If it's not enabled, PHP will silently cast other scalar types when you call the function, which is surprising. (I'm posting this because I was just bitten by it -- although in my case strict_types wouldn't have helped, because the call to the function was from within PHPUnit.)
See also the PHP bug: https://bugs.php.net/bug.php?id=65119
The problem of finals is still interfering with testing work. (This time it's unit tests for FileBackend, where I can't test that it's properly forwarding to LockManager because the LockManager methods are marked final.)
In addition to the advantages of type hints that have already been mentioned, I'll point out that the language is moving toward more and better type hints. PHP 7.4 has typed class properties, and the next version looks like it will have union types, probably followed by a "mixed" type.
Oct 28 2019
Remaining non-deprecated public static methods on Language:
Oct 27 2019
As far as I can tell, the issue is we need a way for IP address and cookies to be available for logging and security checks. We obviously are not going to pass these all the way down the call chain. This seems like exactly the use-case a service manager is intended for. Why not just make a service that contains the needed info? Generally we can just inject the info from the service, but if some code wants to perform certain actions that should be considered relevant to a different IP address/set of cookies, it can inject different info. If something is being performed without relevance to any specific IP address, like a maintenance script run via a cron job, it can use 127.0.0.1 as the IP address (which I imagine is the info PHP supplies anyway). And of course empty cookies.
Oct 25 2019
Nope, config was fine, it was a filtering proxy. The proxy had been configured to allow SSH to the old IP, but needed to be updated to allow the new IP. I should always remember to check for that first before bothering anyone else about issues that could be network-related, but sometimes I forget. :/
Sorry about that, it was a problem on my end after all. All fixed now.
Oct 24 2019
Coverage for includes/libs/lockmanager: 16.64% of lines (116/697)
Oct 23 2019
Coverage for includes/filebackend/: 41.18% of lines (119/289)
It runs them a while after the patch is already merged, or what? It doesn't seem to run them every time a new changeset is submitted to Gerrit, nor in gate-and-submit, nor in the post-merge run that Jenkins reports. Those are the test runs I see in practice.
Pushing over HTTPS works for the time being as a workaround.
It occurs to me that we need to define rules for changes in stability similar to rules for deprecation and removal. If I want to mark a public interface as internal when it wasn't officially marked as such in the past, does that need a mention in release notes? If so, where -- the deprecation section or somewhere else? Are there any rules for how soon I can remove a method (or otherwise break callers) after declaring it unstable? Is there going to be any equivalent of hard deprecation for declaring things unstable?
Oct 22 2019
How am I supposed to test its impact on Wikibase before merging? How can I run Wikibase tests on a patch I'm submitting to core, if they aren't run automatically?
Sep 1 2019
- void return value for functions, for the same reason we want to type-hint generally
- Convert list() to  just like we converted array()
- Specify visibility of class constants (some are only used internally and should be made protected or private)
It looks like soon we're probably going to require PHP 7.2, so features from 7.1 and 7.2 should be considered as well. These pages are useful:
It is not part of the contract of a cache to cache something, insofar as caches by definition are for things that can be regenerated, and therefore are not designed to guarantee persistency.
Is this actually causing problems? MovePage did have it because destruct would actually get called, but now it seems it doesn't. Why would destruct actually get called on the mock? It seems it doesn't really get called every time an object is destroyed.
Aug 30 2019
For the overloading issue, is there any option of just getting more resources? Maybe using EC2 or similar? We're trying to add lots more tests, and one way or another, we're going to need more resources to run them.
ChangesListSpecialPage is not validating the parameters. It's assuming that the namespace parameter in the query string is a semicolon-delimited list of numbers. The URL here is invalid, and has garbage after the namespace parameter:
This will be parsed to int(1), which is probably correct, but it issues a notice.
Again, there are two concerns: reducing latency, and reducing server load. The two are only related if the servers are actually overloaded and therefore individual jobs take longer. What exactly is our concern here? Latency, server load, or both?
Aug 29 2019
It seems that what you're suggesting is that instead of defining a wrapper class that you can use if you want to mock the file access, each class should define separate methods on an ad hoc basis to wrap the file access. Wouldn't it be preferable to define the wrappers only once?
It's been seven days, and seemingly no objections. It would be useful for me to have +2 rights now just to re-submit patches I've written that failed gate-and-submit due to flaky tests or non-substantive reasons ("HHVM doesn't like syntax X"). I seem to have a lot of those, and right now I have to wait for someone else to deal with it.
Since the point of a cache is to save disk reads, I think we would want to directly test that it doesn't do unnecessary disk risks. Otherwise the cache isn't doing its job. Just injecting the directory doesn't give you any way to directly test that the cache is saving disk reads, i.e., that it's behaving usefully.
If you want to split things up, split up PHPUnit tests themselves into multiple chunks. That will allow reducing latencies more or less arbitrarily, assuming we have adequate server resources and we don't spend too much time in setting up the test instance.
Why did phan not catch this?
Aug 28 2019
For that matter, testing anything in that method requires making assumptions about the contents of files in the filesystem and perhaps copy-pasting something about their contents. This doesn't seem like the best approach. The correctness of Language::getGrammarTransformations() shouldn't have to depend on the data that we happen to ship.
So here's an example: Language::getGrammarTransformations() will throw an exception if it finds invalid JSON in a grammar transformations file. The file path is hardcoded to __DIR__ . "/data/grammarTransformations/$languageCode.json" and accessed with is_readable() and file_get_contents(). How are we supposed to test that the exception is thrown properly?
If we wanted to make this feature work properly, I think the right way would be to parse the HTML page content for links, find all links to pages on the current wiki, run a DB query to see how big they are, and add a suitable class to the parsed HTML. This would not affect caching; would have no performance impact on anyone who has the feature disabled; and for whoever has the feature enabled, will add one HTML parse/serialize and one (small, indexed) database query.
Maybe it doesn't really need to be mocked, then. I'll reopen if I find a case where it really does.
Aug 27 2019
I think the best returns here are going to be not on pinpointing specific tests, but looking for systemic issues. Even if you find a test that takes ten seconds, optimizing it will save at most ten seconds, and there aren't enough such tests to add up to the target savings of 10+ minutes.
Aug 26 2019
So let's say for LBFactory, how would you suggest making it not depend at all on the current request? How should it get the request's IP address and User-Agent so that ChronologyProtector works?
By the way, @Ladsgroup, how did you actually measure the performance hit here? I'm going to need a way to test it to make sure I've fixed the problem before re-committing.
Few services will actually want direct access to the request info, so I'd think it doesn't make sense to make a service for this without specific use-cases that don't make more sense to deal with by other means. For instance, LBFactory needs only a few specific pieces of information from the request, so it doesn't seem necessary to make a whole service to pass in for it.