This key predates MediaWiki's standardised notion of request IDs,
and of PSR-3 and Monolog usage.
Bug: T199607
Change-Id: Ibdd5bf12591761ab45be12ba72943e9d94f678cb
This should make error logs easier to work with through a couple
of ways:
* The stack trace is now complete, instead of missing the first
crucial step, which is often the one used for filtering
purposes and for identifying errors within a given deployed
version of MediaWiki. (E.g. when filtering out an error that is
expected to be fixed by the next release and/or when checking
how prominent an error currently is).
* Logstash reports that report message + trace will not need to be
edited by hand to include the file+line.
* The workflow for Logstash generally follows one of two patterns.
The default is to filter by exception.file (including line number),
which is very sure to catch all possible variants thrown from
the same code, regardless of any variables in the message, but
has the downside of not matching week-over-week consistency due to
file paths (at least for WMF) containing the deployment version.
The other option is to filter by message, which has the risk of
possibly excluding too much if there are multiple unrelated ways
to trigger the issue, but is a sensible second option. This is
usually done by filtering on normalized_message for non-exception
errors, but doesn't work well for exceptions because they contain
the file paths and do so in-between the class and message words,
and thus are not compatible with Logstash's default substring/term
match. The alternative of exception.message is then considered but
is lacking the class/type, which can be fragile.
With this change applied, no editing is needed, and no multiple
approaches need to be considered with the same option.
Either filtering by exception.file as-is, or filtering by
normalized_message as-is, regardless of whether it is an exception
error or other message in another channel, will both work.
Bug: T271496
Change-Id: I5908ed53f9b97b3c9cde126aca89ab6fc197c845
A PHP fatal error from MWExceptionRenderer::output() causes the
exception handler to be re-entered, with the original exception thrown
away. So, catch all throwables. That way we can see both error messages,
neither is thrown away.
Bug: T263911
Change-Id: Ie98438cbdd328fe295c9b5202d79edb0c8fb41c5
$file and $line are reported as possibly undefined in static code
analyzer, but the $level = 0 does not pass self::$fatalErrorTypes,
just return earlier
Value 0 is not used:
https://www.php.net/manual/en/errorfunc.constants.php
Change-Id: Ib8431d6ed496ee50447c8c976afe17ebac03e156
* Always use trigger_error for deprecation warnings, not just in development.
They are still silent from the run-time perspective (not thrown as
exceptions).
Previously this code path was only called when $wgDevelopmentWarnings
is set to true. For most dev environments and for CI, this means
nothing much changes given that DevelopmentSettings.php set this to true.
* In the code path that handles native PHP warnings, when setting the $file
and $line attribution that Logstash/Kibana report as "exception.file"
use the same offset as the one that wfDeprecated() has computed from
the back trace. This means it no longer (wrongly/uselessly) attributes
all deprecation warnings to MWDebug.php.
* Trim the message suffix from "Called from <method> in <file>" to
just "Called from <method>". This reduces noise and makes the message
more stable over multiple MW branches. The stack trace is still there
like before.
== Before (only with $wgDevelopmentWarnings) ==
> PHP Deprecated: Use of wfGetScriptUrl was deprecated in MediaWiki 1.35.
> [Called from MediaWiki::__construct in /var/mediawiki/includes/MediaWiki.php at line 67]
>
> Error from line 393 of /var/mediawiki/includes/debug/MWDebug.php
>
> #0 [internal function]: MWExceptionHandler::handleError()
> #1 /var/mediawiki/includes/debug/MWDebug.php(393): trigger_error()
> #2 /var/mediawiki/includes/debug/MWDebug.php(297): MWDebug::sendMessage()
> #3 /var/mediawiki/includes/debug/MWDebug.php(270): MWDebug::sendRawDeprecated()
> #4 /var/mediawiki/includes/GlobalFunctions.php(1032): MWDebug::deprecated()
> #5 /var/mediawiki/includes/GlobalFunctions.php(2548): wfDeprecated()
> #6 /var/mediawiki/includes/MediaWiki.php(67): wfGetScriptUrl(string)
> #7 /var/mediawiki/load.php(50): MediaWiki->__construct()
== After (always) ==
> Use of wfGetScriptUrl was deprecated in MediaWiki 1.35. [Called from MediaWiki::__construct]
>
> Error from line 67 of /var/mediawiki/includes/MediaWiki.php
>
> #0 [internal function]: MWExceptionHandler::handleError()
> #1 /var/mediawiki/includes/debug/MWDebug.php(293): trigger_error()
> #2 /var/mediawiki/includes/debug/MWDebug.php(270): MWDebug::sendRawDeprecated()
> #3 /var/mediawiki/includes/GlobalFunctions.php(1038): MWDebug::deprecated()
> #4 /var/mediawiki/includes/GlobalFunctions.php(2548): wfDeprecated()
> #5 /var/mediawiki/includes/MediaWiki.php(67): wfGetScriptUrl(string)
> #6 /var/mediawiki/load.php(50): MediaWiki->__construct()
Bug: T252923
Change-Id: I1d4a166b6dff8b0e19fce3fab409f4a89e734ee6
This reminder, while useful to see on a web page during local
development or when live upgrading a small wiki, is not so
useful in error logs and production monitoring.
In addition to being general noise and boilerplate to have to
know to ignore (and to know where to look instead), it has the
unfortunate side-effect of letting the 255-char trimmed normalized
messages containing none of the actual errors.
Remove this from the DBQueryError exception message, and place
it instead in the code paths of:
* MWExceptionRenderer::getHTML, used when a fatal error
happens but we are still able to render it in a skinned
output page.
Test Plan:
- Edit SpecialBlankpage.php#execute, and add the following,
which involves a non-existant 'pagex' table:
$db = wfGetDB( DB_REPLICA );
$db->select( 'pagex', '1', 'foo = 2', __METHOD__ );
- View Special:Blankpage on your wiki
* MWExceptionRenderer::output (main 'else' branch), used
when a fatal error is unable to recover and thus render
a plain text HTML response.
Test Plan:
- Edit SpecialBlankpage.php#execute, and add the following,
which involves an SQL syntax error:
$db = wfGetDB( DB_REPLICA );
$db->select( 'page', '1', 'foo =/ 2', __METHOD__ );
- View Special:Blankpage on your wiki
Bug: T255202
Change-Id: Ie08199ced767486f9e049934a334a1438f266aa6
No behavioural change. This change in abstraction is prep
for I1d4a166b6df and makes that diff cleaner.
Change-Id: If3f5836fcb2fc0c16aa1c3bdc1333d8c8f892f3b
Migrate all callers of Hooks::run() to use the new
HookContainer/HookRunner system.
General principles:
* Use DI if it is already used. We're not changing the way state is
managed in this patch.
* HookContainer is always injected, not HookRunner. HookContainer
is a service, it's a more generic interface, it is the only
thing that provides isRegistered() which is needed in some cases,
and a HookRunner can be efficiently constructed from it
(confirmed by benchmark). Because HookContainer is needed
for object construction, it is also needed by all factories.
* "Ask your friendly local base class". Big hierarchies like
SpecialPage and ApiBase have getHookContainer() and getHookRunner()
methods in the base class, and classes that extend that base class
are not expected to know or care where the base class gets its
HookContainer from.
* ProtectedHookAccessorTrait provides protected getHookContainer() and
getHookRunner() methods, getting them from the global service
container. The point of this is to ease migration to DI by ensuring
that call sites ask their local friendly base class rather than
getting a HookRunner from the service container directly.
* Private $this->hookRunner. In some smaller classes where accessor
methods did not seem warranted, there is a private HookRunner property
which is accessed directly. Very rarely (two cases), there is a
protected property, for consistency with code that conventionally
assumes protected=private, but in cases where the class might actually
be overridden, a protected accessor is preferred over a protected
property.
* The last resort: Hooks::runner(). Mostly for static, file-scope and
global code. In a few cases it was used for objects with broken
construction schemes, out of horror or laziness.
Constructors with new required arguments:
* AuthManager
* BadFileLookup
* BlockManager
* ClassicInterwikiLookup
* ContentHandlerFactory
* ContentSecurityPolicy
* DefaultOptionsManager
* DerivedPageDataUpdater
* FullSearchResultWidget
* HtmlCacheUpdater
* LanguageFactory
* LanguageNameUtils
* LinkRenderer
* LinkRendererFactory
* LocalisationCache
* MagicWordFactory
* MessageCache
* NamespaceInfo
* PageEditStash
* PageHandlerFactory
* PageUpdater
* ParserFactory
* PermissionManager
* RevisionStore
* RevisionStoreFactory
* SearchEngineConfig
* SearchEngineFactory
* SearchFormWidget
* SearchNearMatcher
* SessionBackend
* SpecialPageFactory
* UserNameUtils
* UserOptionsManager
* WatchedItemQueryService
* WatchedItemStore
Constructors with new optional arguments:
* DefaultPreferencesFactory
* Language
* LinkHolderArray
* MovePage
* Parser
* ParserCache
* PasswordReset
* Router
setHookContainer() now required after construction:
* AuthenticationProvider
* ResourceLoaderModule
* SearchEngine
Change-Id: Id442b0dbe43aba84bd5cf801d86dedc768b082c7
Make sure that CAUGHT_BY_HANDLER is only for errors caught by the
handler from MWExceptionHandler::installHandler().
Add CAUGHT_BY_ENTRYPOINT constant for entrypoint try/catch logic,
e.g. MediaWiki::run()/ApiMain::executeActionWithErrorHandling().
Use Throwable to catch more types of errors given that PHP 7.2
is already required.
Change-Id: Ib496e26572c98d771a772972676c02c05b872e31
PHP 7.0 makes many error conditions throw instances of the new Error class
which does not extend the known Exception.
The Throwable interface provides a concise and type-safe way of handling
either, e.g. for logging purposes, but HHVM did not support it, requiring
tedious fallback checks.
This commit replaces occurrences of Exception in code paths equally
covered by Throwable, like Exception|Throwable parameter and return types
(also nullable), instanceof guards, duplicated `catch` blocks, as well as
related comments and documentation blocks, with the exception of $previous
parameter descriptions consistent with the manual at
https://www.php.net/manual/en/exception.construct.php
Proper type declarations have been added or reinstated where possible.
Change-Id: I5d3920d3cc66936a350314e2f19c4f6faeffd7c0
Follows-up 115df551f2 and its CR (Gerrit I664bfe55359aadb343ee7).
The comment was out of date.
* The stack is indeed unwound at this point for most use cases,
but using debug_backtrace() doesn't help that.
What debug_backtrace sees as the same as '(new Exception)->getTrace()'.
The reason the code used debug_backtrace isn't to obtain a better
trace. It was using that because HHVM gave us a magic non-standard
'$trace' parameter into the callback. Given there is no way to
construct the built-in Exception object with a custom trace,
the only way we could log it was to build the rest of the array
ourselves as well. We'd fallback for PHP 5 by back-filling only
the trace part using debug_backtrace().
The code for HHVM has been removed meanwhile (d1679955b4).
What's left is standard ErrorException handling, which our
Monolog layer should be formatting instead, not duplicated
here.
Getting a trace here would be nice, but is out of scope for this
change. Assuming the php-wmerrors handler (php7-fatal-error.php
at WMF) sees the stack, that's where we'll get it for prod.
Within MW core under plain PHP 7 there simply isn't a way to get
it, hence php-wmerrors exists (covered in more detail at T187147,
specifically T187147#5165179).
Bug: T233342
Change-Id: Ic81076a8fd1a593460528162d4319fdedb985f30
This make it easier to write queries in Logstash given the limited
capabilities of its user interface, so that filters for 'exception.trace'
will also match those of fatal errors.
This has gotten even more confusing in the last week because the fatal
errors logged by /etc/php/php7-fatal-error.php in production did use
'exception.trace' already and that means that the same exact exception
(e.g. "Allowed memory … exhausted" and "Maximum execution time … exceeded")
would sometimes be under 'fatal_exception' and sometimes under 'exception'.
Bug: T233342
Change-Id: I664bfe55359aadb343ee742f59af5f26f4c19339
With the removal of HHVM support, we can remove HHVM-specific error
handling from MWExceptionHandler.
* Remove references to HHVM from comments.
* Remove references to PHP 7/PHP 7+ from comments, as we now require
PHP 7+.
* Remove support for the HHVM-exclusive $trace parameter from
MWExceptionHandler::handleFatalError().
* Remove HHVM-specific regex when checking if the current error
is a class not found error.
* Remove delegation to MWExceptionHandler::handleFatalError() from
MWExceptionHandler::handleError(), as it was only used for HHVM
fatals when running in PHP 5 mode.
* Remove MWExceptionHandler::$handledFatalCallback that was protecting
against running MWExceptionHandler::handleFatalError() twice, since this
could only happen if MWExceptionHandler::handleError() called
MWExceptionHandler::handleFatalError().
* Remove HHVM's FATAL_ERROR constant from MWExceptionHandler::$fatalErrorTypes.
Change-Id: I110f7195c3094e761264d382f3a26e9a425bb8ad
These are reported by phan as PhanTypeMismatchArgumentNullableInternal
when null_casts_as_any_type is disabled.
Change-Id: I85076ee31c1bfc59a19600e84da0d915e425890a
It is now clear to me why most fatals are logged to 'exception'
on PHP 7, instead of 'fatal' (as on HHVM). It is because these
are, as of PHP 7, technically recoverable if caught locally
with 'catch (Throwable)', and as such should no longer be classified
as 'fatal'. I suppose that's fine and something we'll get used to.
The most important distinction to keep is between 'error' and 'fatal/exception'
given the latter is more heavily monitored and alerted on, but
otherwise they are not usually distingished in query, we treat
them equal for the most part.
Bug: T187147
Change-Id: I64bf0b32dd2648cf72297bdc294e315375329a4d
Follows-up ef06b528d9, which fixed the severity of all PHP errors
to have severity "Warning" or higher.
However, it also accidentally changed the label of "Notice"
to "Warning", which is confusing and incorrect.
Change-Id: Iffd39aa23b7f2cbff5cdaf876b8d4d595dcd6f96
Things like "Catchable fatal error: Must be X, null given" stop
execution immediately after the error handler callback, and produce
an HTTP 500 Internal Server Error page. They are very fatal.
Per <https://secure.php.net/manual/en/language.errors.php7.php>,
on PHP 7 this results in a TypeError throwable which will
eventually fatal the request and be reported through
set_exception_handler, or be caught by our top-level 'catch'
and artificially forwarded to our set_exception_handler callback.
On HHVM, these fatal error types are PHP5-like in that they
don't have a throwable object to throw yet, instead the error
meta-data is sent directly as parameeters to set_error_handler,
the same as for warnings. We need to intercept them there so
that they are reported correctly.
Sample from PHP 7:
> MediaWiki\Storage\MutableRevisionRecord::newFromParentRevision( null );
> [no req] TypeError from line 50 of .../MutableRevisionRecord.php:
> Argument 1 passed to ...\MutableRevisionRecord::newFromParentRevision()
> must be an instance of MediaWiki\Storage\RevisionRecord, null given,
> called in /var/www/mediawiki/maintenance/eval.php(78) ...
[exit status: error(255)]
Sample from HHVM:
> MediaWiki\Storage\MutableRevisionRecord::newFromParentRevision( null );
> [hphp] set_error_handler called with:
> ... $type = 4096 // E_RECOVERABLE_ERROR
>
> [hphp] [...] Catchable fatal error:
> Argument 1 passed to ...\MutableRevisionRecord::newFromParentRevision()
> must be an instance of MediaWiki\Storage\RevisionRecord, null given
> ...
[exit status: error(255)]
Interestingly, if you forget to return false from set_error_handler
for fatal errors on HHVM, it can actually continue execution.
Bug: T205677
Change-Id: I18dd2ff37fa2c2679d0c598cbeff0c61c2fe0253
All PHP errors should be considered by monitoring queries
and error-rate alerts.
Levels DEBUG and INFO are for manual entries that can help in
debugging to see what path a program went down or what the
circumstances were in that code.
There is no reason to spread PHP error-types out on the full range
of PSR-3's DEBUG to ERROR spectrum. Instead, keep it within either
WARNING or ERROR, not below it.
Change-Id: I3f35a519b50aef5b93b9ab7a89a7c3e11d70681f
Uses new PHP 5.6 syntax like ...parameter unpacking and
calling anything looking like a callback to make the code more readable.
There are much more occurrences but this commit is intentionally limited
to an easily reviewable size.
In one occurrence, a simple conditional instead of trickery was much more readable.
This patch finishes all the easy stuf in the core, the remainder is either unobvious
or would result in smaller readability gains. It will be carefully dealt with in
further commits.
Change-Id: I79a16c48bfb98b75e5b99f2f6f4fa07b3ae02c5b
Use human-readable stack trace instead of array dump,
try to display the URL and the request ID, use the same
message format as exceptions,
Bug: T189851
Change-Id: I3edf2dbd5639ceecc668719c065ecdce33157ff5
Add a $wgPropagateErrors configuration variable which can be used
to prevent passing handled errors to PHP (and thus logging them twice).
Bug: T45086
Change-Id: I64ab09762a04de2007b7d7864e3c504a1d6f8aee
phpdbg is a gdb-style debugger for PHP that is run from the command
line. However, it has a different PHP_SAPI value, so it was impossible
to run maintenance scripts with it (until now).
To avoid having to check both PHP_SAPI values in a bunch of places,
introduce wfIsCLI() to easily check whether running from the
command-line or not.
We're (CI team) interested in generating code coverage with phpdbg
instead of xdebug, hence this patch.
Bug: T184043
Change-Id: Id1f994ca146d7858cd8bb6ab6cdbb7718ff524fb
Clean up use of @codingStandardsIgnore
- @codingStandardsIgnoreFile -> phpcs:ignoreFile
- @codingStandardsIgnoreLine -> phpcs:ignore
- @codingStandardsIgnoreStart -> phpcs:disable
- @codingStandardsIgnoreEnd -> phpcs:enable
For phpcs:disable always the necessary sniffs are provided.
Some start/end pairs are changed to line ignore
Change-Id: I92ef235849bcc349c69e53504e664a155dd162c8
I registered the additional shutdown function for CLI only
as it shouldn't have effect otherwise.
Bug: T177414
Change-Id: I440d294eef5e307743cfc7f5ab3b531e8c973873
Let Monolog insert exception_id and exception_url so that consumers
of logging data (such as Logstash) may provide a normalised message
that does not contain these variants, for ease of aggregation
and message trend counting.
Bug: T45086
Change-Id: I7cc8f8c9e68031ad6771593d390079c0a3a535b9
In phpcs.xml rename renamed sniffs and add the failing sniffs,
because now the whole sniff is no longer excluded.
Change-Id: If5b0bd16028761abc2c47ace9e97d37ad14bb36f
Remove the exit(1), which does not seem to be needed by any callers.
Doing so means that post-send updates can still happen, such as the
pushing of lazy jobs.
Better avoid showing exceptions in doPostOutputShutdown(), given
that an error may have already been shown. By the post-send part,
it's to late to show errors anyway.
Bug: T100085
Change-Id: Ib1c75323f222a0e02603d6415626a4b233e8e1c7
MWExceptionHandler::rollbackMasterChangesAndLog() only logged exceptions
if there were already master changes. This is extremely problematic when
debugging, especially in situations like DeferredUpdates where they were
silently being swallowed.
This makes it log exceptions in all paths, erring on the side of logging
the same exception twice (theoretically it's possible I suppose) instead
of not at all.
Also make the method able to handle DBError exceptions, which most of
the callers seemed to be assuming. ApiMain was handling this explicitly.
Bug: T168347
Change-Id: I8739051f824a455ba669344184c3b11ac95cb561
It's unreasonable to expect newbies to know that "bug 12345" means "Task T14345"
except where it doesn't, so let's just standardise on the real numbers.
Change-Id: I6f59febaf8fc96e80f8cfc11f4356283f461142a
This distinguishes random errors caught and explicitly logged by various
callers from those that were only caught by the registered exception handler.
The later are likely to have more visibily impact on the user.
Change-Id: Icb4c4e9376270c5475c95cf40708a7ca3e4e0a49
When we create an exception-like set of data for logging in
MWExceptionHandler::handleFatalError, add it to the logging context as
a 'fatal_exception' member. The 'exception' value in a log context
should only be populated with a real Exception object.
Bug: T150106
Change-Id: I253943849f19ed5480dbda7bfbc0bf607f69c47d
* Make isTransactableQuery() exclude CREATE/ALTER.
Starting transactions for schema changes like this can cause
errors as it is not supported for MySQL and some Postgres
operations. Note that temporary tables are session-level,
so they are not effected by this change.
* Clean up the transaction logic in determineCoreSchema()
so a transaction is not left dangling.
* Fix broken getSchemaPath() call in PostgresInstaller.
* Avoid warnings in DatabasePostgres::closeConnection() if
mConn is already unset.
* Commit master changes in doMaintenance.php before running
deferred updates, just as MediaWiki.php does.
* Change E_WARNING to E_USER_WARNING to avoid notices in the
default /rdbms error handlers.
* Also avoid trying to rollback in MWExceptionHandler if the
LBFactory service is disabled, which just results in an error.
Bug: T147599
Change-Id: I64ccab7f9b74f60309ba0c9a8ce68337c42ffb0f
* Actually use MWExceptionRenderer::AS_RAW. Use this after
an error is thrown while trying to pretty render the original
error. This is how this case was originally handled before.
* Do not show the google form or file cache in CLI mode.
Change-Id: I130499753efbf8b4d6d254ea36bacb2473952c1b
Follow up to 00bee0297. Many MWException subclasses override
MWException::report to do things like special logging and setting the
HTTP response status code. We need to keep calling those methods until
MWExceptionRenderer knows how to handle all of them.
Bug: T147098
Change-Id: I2c90e2d9e9b019357458c7e14a3d602b591c6f5b
* This handles the work of showing exceptions so that
MWException does not have too.
* Simplify the DBError classes to regular Exception
classes. Lots of pointless prettification has been
removed, but DBConnectionError still gets the usual
special treatment of a fallback page and Google form.
* Remove hacky file cache fallback code that probably
did not work.
* Make MWExceptionHandler::report() wrap
MWExceptionExposer::output().
* Make MWException::runHooks() wrap
MWExceptionExposer::runHooks().
Change-Id: I5dfdc84e94ddac65417226cf7c84513ebb9f9faa
* Also make ErrorPageError exceptions display themselves
in PRESEND mode. Before they were always suppressed.
* Make DataUpdate::runUpdates() simply wrap
DeferredUpdates::execute().
* Remove unused installDBListener() method, which was
basically moved to Maintenance.
* Enable DBO_TRX for DeferredUpdates::execute() in CLI mode
* Also perform sub-DeferrableUpdate jobs right after their
parent for better transaction locality.
* Made rollbackMasterChangesAndLog() clear all master
transactions/rounds, even if there are no changes yet.
This keeps the state cleaner for continuing.
* For sanity, avoid calling acquirePageLock() in link updates
unless the transaction ticket is set. These locks are
already redundant and weaker in range than the locks the
Job classes that run them get. This helps guard against
DBTransactionError.
* Renamed $type to $stage to be more clear about the order.
Change-Id: I1e90b56cc80041d70fb9158ac4f027285ad0f2c9
