The name change happened some time ago, and I think its
about time to start using the name name!
(Done with a find and replace)
My personal motivation for doing this is that I have started
trying out vscode as an IDE for mediawiki development, and
right now it doesn't appear to handle php aliases very well
or at all.
Change-Id: I412235d91ae26e4c1c6a62e0dbb7e7cf3c5ed4a6
* Convert to a single map.
* Simplify source code.
* Remove deprecation warning for something we're not likely to
remove. This was embedded into LocalSettings.php files and
should generally just keep working and is easy and cheap to do.
Also note clear why one half warned and the other half didn't
(because that's the one we happen to use in prod.)
* Simplify the tests. A lot of the boilerplate was no longer needed.
* Reduce abstraction in the test as was was not more complex
than the source it tests.
Change-Id: If3e7e25dbf3bb408581fc16ac8e556b44b1855ad
Deprecating something means to say something nasty about it, or to draw
its character into question. For example, "this function is lazy and good
for nothing". Deprecatory remarks by a developer are generally taken as a
warning that violence will soon be done against the function in question.
Other developers are thus warned to avoid associating with the deprecated
function.
However, since wfDeprecated() was introduced, it has become obvious that
the targets of deprecation are not limited to functions. Developers can
deprecate literally anything: a parameter, a return value, a file
format, Mondays, the concept of being, etc. wfDeprecated() requires
every deprecatory statement to begin with "use of", leading to some
awkward sentences. For example, one might say: "Use of your mouth to
cough without it being covered by your arm is deprecated since 2020."
So, introduce wfDeprecatedMsg(), which allows deprecation messages to be
specified in plain text, with the caller description being optionally
appended. Migrate incorrect or gramatically awkward uses of wfDeprecated()
to wfDeprecatedMsg().
Change-Id: Ib3dd2fe37677d98425d0f3692db5c9e988943ae8
There is native support for all of this now in PHP, thanks to changes
and additions that have been made in later versions. There should be no
need any more to ever use call_user_func() or call_user_func_array().
Reviewing this should be fairly easy: Because this patch touches
exclusivly tests, but no production code, there is no such thing as
"insufficent test coverage". As long as CI goes green, this should be
fine.
Change-Id: Ib9690103687734bb5a85d3dab0e5642a07087bbc
The one caller of LBFactory::getChronologyProtectorTouched() was calling
it with a domain ID instead of a master server name. Using the master
server name to identify replication position makes sense for
ChronologyProtector, since the replication position may be reset when
the master changes, but it is an odd convention for LBFactory. So:
* Rename all $dbName variables in ChronologyProtector to $masterName,
for clarity.
* Interpret the first parameter to
ILBFactory::getChronologyProtectorTouched() as a database domain, to
make its only existing caller work.
* Change the first parameter to ChronologyProtector::getTouched() from a
string to a strongly typed ILoadBalancer, by analogy with
applySessionReplicationPosition(). This removes the master name concept
from the public interface.
* Mark ChronologyProtector @internal. The accessor in LBFactory is
protected, so extensions can't use it anyway. This is just to clarify
why I think changing the parameter to getTouched() without b/c is OK.
* Add a simple test which mostly just checks that ChronologyProtector gets
called with the correct parameters. It's an
LBFactory/ChronologyProtector integration test.
Change-Id: I3b4832b5a4d7410e94b9c51577b30b31d49bc63d
Done with `composer fix` and suppressing the rest (i.e. sniffs for
global variables, which for core should be suppressed anyway).
Additionally, add `-p` to `phpcbf`, as otherwise it just seems stuck.
Change-Id: Ide8d6cdd083655891b6d654e78440fbda81ab2bc
This allows full configuration of the LoadMonitor, not just setting
the class name like loadMonitorClass allows. Deprecate the later.
Also fix obsolete LoadBalancer constructor documentation and
standardize the style of LBFactoryMulti::__construct() comments.
Change-Id: Icfdc0d6bbf227dce56e65ad679b93ce3f604e9f7
Add several new internal methods to help with wrangling
the various formats that rows, conditions, options, and
unique key lists can come in. Remove now unused method
isMultiRowArray().
Add various sanity checks and logging for parameters to
upsert(), replace(), insert(), and insertSelect().
Move DatabasePostgresTest to the integration/ directory.
Change-Id: If5988a6f0816e8da2cbf2fd612e1a3e3a2e9c52f
Add new ILBFactory::setDomainAliases() method for injection database
domain aliases and call it in MWLBFactory::setDomainAliases().
Also:
* Remove overkill "last db/section" caching in LBFactoryMulti
* Clean up some LBFactoryMulti code comments
* Split out separate MWLBFactoryTest test file
Change-Id: If180a58c61178969ca7587c4a06b8786574c7254
Add a query builder class which encapsulates the parameters to
IDatabase::select() and related functions.
Override useIndexClause() and ignoreIndexClause() in DatabaseTestHelper
so that index hints can be tested.
Bug: T243051
Change-Id: I58eec4eeb23bd7fb05b8b77d0a748f1026afee52
*assertType is marked as deprecated, and should ideally be removed soon
(i.e. no hard deprecation to follow)
*Most usages of assertType in core were autofixed by using I8ef556b630812aeea77c5606713f53d9af609f1b
*assertTypeOrValue was removed because only used in SiteTest
(codesearch: https://codesearch.wmflabs.org/search/?q=assertTypeOrValue&i=nope&files=&repos=)
*SiteTest::assertTypeOrFalse was removed because unused
Bug: T192167
Change-Id: Icb3014b8fe7d1c43e64a37e0bdaaffec18bb482f
Move the DBO_TRX init logic out of Database::__construct() and into
LoadBalancer since the later already handles setting and clearing this
flag based on transaction rounds starting and ending.
Add 'lazyMasterHandle', 'topologyRole', and 'topologicalMaster' parameters
to Database::factory() and inject them via LoadBalancer all at once in order
to avoid worrying about call order. Move some type casting code to
Database::__construct().
Add IDatabase::getTopologyRole()/getTopologicalMaster().
Use constants for getLBInfo()/setLBInfo() for better usage tracking and
typo resistance.
Change-Id: I437ce434326601e6ba36d9aedc55db396dfe4452
Add public, protected or private to function missing a visibility
Enable the tests folder for the phpcs sniff
Change-Id: Ibefce76ea9984c47e08c94889ea2eafca7565e2c
assertEquals( null, … ) still succeeds when the actual value is 0, false,
an empty string, even an empty array. All these should be reported as a
failure, I would argue.
Note this patch previously also touched assertSame( null ). I reverted
these. The only benefit would have been consistency within this codebase,
but there is no strict reason to prefer one over the other. assertNull()
and assertSame( null ) are functionally identical.
Change-Id: I92102e833a8bc6af90b9516826abf111e2b79aac
assertSame() is guaranteed to never do any magic type conversion.
This can be critical when accidentially comparing empty strings (a
value PHP considers to be "falsy") to false, 0, 0.0, null, and such.
Change-Id: I2e2685c5992cae252f629a68ffe1a049f2e5ed1b
assertSame() is guaranteed to not do any type conversion. This can be
critical when acciden tially comparing, for example, 0 to 0.0.
Change-Id: Iffcc9bda69573623ba14af655dcd697d0fcce525
Enforce this pattern for the remaining LoadBalancer methods.
Carry this over into Database::close() to decide how loud the
error handling should be.
In LBFactory, clean up ownership of newMainLB()/newExternalLB().
The should have a null owner if called from outside the class
since the LBFactory does not track nor care about them anymore
in that case. Disable newMainLB() for LBFactorySingle as it
makes no sense and was broken.
Also remove some redundant abstract LBFactory methods that
just duplciate ILBFactory.
Bug: T231443
Bug: T217819
Depends-On: I7ed5c799320430c196a9a8e81af901999e2de7d0
Change-Id: I90b30a79754cfcc290277d302052e60df4fbc649
Make LoadBalancer::reallyOpenConnection() handle initializing DBO_TRX
instead of Database::__construct().
Also:
* Avoid having the "catch" block appear like it returns a
half-constructed Database.
* Use the variable name $conn instead of $db to be consistent
throughout the class. Only send Database::__construct() parameters
that it recognizes instead of mixing in setLBInfo() data.
Change-Id: Iffc3d1d0713051a164adb51a4c4ee12e4ac887c3
Rename Database::hasFlags() to sound less similar to getFlag()/setFlag().
The order of class fields is roughly:
* Objects and resources
* Configuration
* Mutable options that do not have to be kept in sync with the connection
* Session level state tracking (should be in sync with the connection)
* Transaction level state tracking (should be in sync with the connection)
* Results from or timestamp of the last time a certain event occurred
Change-Id: Ibdca2fefe7ed2c792344c5602b5191a950eed933
After approval of RFC T191231, we are going to drop oracle and mssql
and it will be possible to bring back the support using the abstract schema
Adding to release notes will be done in a follow-up
Bug: T230418
Change-Id: I90bd5cfcc3e18011b193c965fdb1fa54675040b5
Define missing DatabaseSqlite::doSelectDomain() method to handle attempts
to change the database, prefix, and/or schema.
Also add sanity check to serverIsReadOnly() to make sure open() was called
Change-Id: I72c25bf4dab5e01def3fb9472217e7637aede1d4
If the specified default group does not have a corresponding server load map
defined, then ignore it and use GROUP_GENERIC.
Also, consolidate the group load and group reader index code for simplicity
Change-Id: Ic8bf9a3ebcbffb81fb14d7b1787a2adb97ac525d
This does what ChronologyProtector wants more rigorously and is better
named. Not all replica servers will have the same position, so they
should be compared to get the highest one.
Simplify the getMasterPos() method to only return master positions
as the other current callers do not need anything else. It will now
connect if needed as well. This should make the method naming better.
Reducing the use of replica derived replication postitions (instead
of those from the master) makes certain GTID issues less likely,
such as the matter of obsolete domain IDs.
Increase general test coverage of LoadBalancer.
Bug: T224422
Change-Id: I5420721ee339a24d09c26c38709500c7bbe797c2
Add and use IDatabase::getServerConnection() method to avoid loops caused
caused by pickReaderIndex() calling getConnection() for the master server.
That lead to getReadOnlyReason() triggering pickReaderIndex() again.
Make getLaggedReplicaMode() apply when the master has non-zero load and
the replicas are all lagged.
Remove "allReplicasDownMode" in favor of checking getExistingReaderIndex()
instead. This reduces the amount of state to keep track of a bit.
Follow-up to 95e2c99094
Bug: T226678
Bug: T226770
Change-Id: Id932c3fcc00625e3960f76d054d38d9679d25ecc
Split out private resolveGroups() method for normalizing query
groups. Move some server index validation to getConnectionIndex()
and make it stricter.
Make getConnectionIndex() group fallback logic aware of the generic
group. The main use case for custom default groups is heavy scripts
or jobs that run in the background. It is probably better to have
good DB server redundancy for the query group and rely on it rather
than possibly fallback onto all of the main servers used for normal
requests. The later behavior could spread slowness or outages.
Also make getAnyOpenConnection() more robust and readable by
splitting out a private pickAnyOpenConnection() method that
checks IDatabase::trxLevel().
In addition, remove redundant is_int() check from isOpen()
method as it will return false in that case even without it.
Remove bogus getConnection() parameter in testCopyTestData().
Change-Id: Ica619c5487c761c724791d151db7388e4b41b0aa
Following discussion in Ibb8175981092d7f41864e641cc3c118af70a5c76, this patch
proposes to further reduce the scope of what unit tests may access, by removing
the loading of DefaultSettings and GlobalFunctions.php. This also has the
implied effect of disabling the storage backend, as well as the global service
locator.
MediaWikiTestCase is renamed to MediaWikiIntegrationTestCase so it's scope and
purpose is more clear. Whether we still need to keep `@group Database`
annotation around is debatable, as it's unclear to me what the performance costs
are of implying database access for all tests which extend IntegrationTestCase.
As far as I can tell, `@group Database` is primarily used in CI to run faster
tests before slower ones, and with the new UnitTestCase the annotation seems
redundant.
To run all testsuites, use `composer phpunit`. Other composer scripts:
- `composer phpunit:unit` to run unit tests
- `composer phpunit:integration` to run integration tests
- `composer phpunit:coverage` to generate code coverage reports from unit
tests (requires XDebug).
Note that you can pass arguments to composer scripts with `--`, e.g. `composer
phpunit:integration --exclude-group Dump`.
Other changes:
- Rename bootstrap.php to bootstrap.maintenance.php so it's clear it's part of
the legacy PHPUnit-as-maintenance-class setup
- Create new bootstrap.php which loads the minimal configuration necessary for
the tests, and do additional setup in the run() method of the unit/integration
test case classes
- Move the unit-tests.xml file to phpunit.xml.dist in preparation for this being
the default test configuration
For a follow-up patch:
- Find unit/integration tests for extensions/skins
- Migrate other test suites from suite.xml
- Support running all tests via vendor/bin/phpunit
Bug: T84948
Bug: T89432
Bug: T87781
Change-Id: Ie717b0ecf4fcfd089d46248f14853c80b7ef4a76
This avoids having two similar fields that have to stay
in sync. Clean up the related error handling for connections.
If a connection handle is unusable, like when essential SET
queries fail, then destroy it.
Also:
* Avoid use of transactions in DatabasePostgres::determineCoreSchema.
* Make sure all subclasses log on connection failure.
* Add schema sanity checks to mysql/sqlite classes.
* Add IDatabase::QUERY_NO_RETRY flag to simplify reasoning about
queries that already run on open() to begin with.
* Remove unused return value of Database::open.
* Remove deprecated Database::reportConnectionError method.
Change-Id: I97beba7ead1523085bda8784234d00c69ef1accc
This is slightly more robust and makes the intent much clearer
than random calling code checking getServerCount() all over the
place. In addition, this yields better separation of concern.
Also, cleanup the LoadBalancer constructer a bit and make the
validation a bit stricter.
Make some server index comparisons strict while at it.
Change-Id: Icc1a35bd65c6862ff81faa3ab9b2aa7cafe29443
This changeset implements T89432 and related tickets and is based on exploration
done at the Prague Hackathon. The goal is to identify tests in MediaWiki core
that can be run without having to install & configure MediaWiki and its dependencies,
and provide a way to execute these tests via the standard phpunit entry point,
allowing for faster development and integration with existing tooling like IDEs.
The initial set of tests that met these criteria were identified using the work Amir did in
I88822667693d9e00ac3d4639c87bc24e5083e5e8. These tests were then moved into a new subdirectory
under phpunit/ and organized into a separate test suite. The environment for this suite
is set up via a PHPUnit bootstrap file without a custom entry point.
You can execute these tests by running:
$ vendor/bin/phpunit -d memory_limit=512M -c tests/phpunit/unit-tests.xml
Bug: T89432
Bug: T87781
Bug: T84948
Change-Id: Iad01033a0548afd4d2a6f2c1ef6fcc9debf72c0d
Add a "readIndexByGroup" field that does the same thing that the
generic read index does except for query grouped connections.
Also remove some pointless checks at the end of getReaderIndex().
Change-Id: I3bd6295be4355ee930960f89ccb07811dd8c5545
