$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
For compliance with the new version of the table interface policy
(T255803).
This patch was created by an automated search & replace operation
on the includes/ directory.
Bug: T257789
Change-Id: I17e5e92e24c708ffc846945a136347670a3a20c7
For compliance with the new version of the table interface policy
(T255803).
This patch was created by an automated search & replace operation
on the includes/ directory.
Bug: T257789
Change-Id: Ie32c1b11b3d16ddfc0c83a757327d449ff80b2e4
For compliance with the new version of the table interface policy
(T255803).
This patch was created by an automated search & replace operation
on the includes/ directory.
Bug: T257789
Change-Id: I5ffbb91882ecce2019ab644839eab5e8fb8a1c5f
For compliance with the new version of the table interface policy
(T255803).
This patch was created by an automated search & replace operation
on the includes/ directory.
Bug: T257789
Change-Id: If560596f5e1e0a3da91afc36e656e7c27f040968
Exceptions classes are nearly always value objects, and should in most
cases by newable.
Bug: T247862
Change-Id: I4faa8ec6ea8bc44086cfc8075b32d10eea61e9df
Per the Stable Interface Policy, PHP interfaces should not be
directly implemented by extensions, unless they are marked to be safe
for that purpose.
Bug: T247862
Change-Id: Idd5783b70fc00c03d57f5b1a887f0e47c4d7b146
This annotates classes that can safely be instantiated by
extensions, per the Stable Interface Policy.
Bug: T247862
Change-Id: Ia280f559874fc0750265ddeb7f831e65fd7d7d6a
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
Add hook interfaces which were generated by a script which parses
hooks.txt and identifies caller namespaces and directories.
Hook interfaces are mostly placed in a Hook/ subdirectory
relative to the caller location. When there are callers in multiple
directories, a "primary" caller was manually selected. The exceptions to
this are:
* The source root, maintenance and tests, which use includes/Hook. Test
hooks need to be autoloadable in a non-test request so that
implementing test interfaces in a generic handler will not fail.
* resources uses includes/resourceloader/Hook
* The following third-level subdirectories had their hooks placed in
the parent ../Hook:
* includes/filerepo/file
* includes/search/searchwidgets
* includes/specials/forms
* includes/specials/helpers
* includes/specials/pagers
Parameters marked as legacy references in hooks.txt are passed
by value in the interfaces.
Bug: T240307
Change-Id: I6efe2e7dd1f0c6a3d0f4d100a4c34e41f8428720
For other endpoints this was already fixed, as all MWExceptionRenderer
logic checks headers_sent() before outputting headers.
For the MW_API condition, it was calling wfHttpError(), which in
turn unconditionally tried to send headers.
Fix this by removing use of wfHttpError(), and instead re-use the
existing logic for a minimal http error page. Do this by removing
the early condition and instead let if fall into the general
render methods, and then treat MW_API as a non-OutputPage scenario.
Bug: T225657
Change-Id: I38bbf8007078c290a2576ef177b789fab1d2059f
$wgLang is usually going to be a stub, since its semi-deprecated
so more and more code-paths don't call it. I'm not exactly sure
what checking it was supposed to accomplish. Note that
MWExceptionRenderer has very similar logic that does not check
$wgLang
Bug: T246619
Change-Id: I101a60b6fb3bc2c1abfa16fd1784caab3f75a95b
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
Accept these objects as params to the UserBlockedError constructor,
since they are used to make the block error message. Pass them from
SpecialPages, Actions and EditPage.
If a caller does not pass all of these params, get them from the
global context as before.
Bug: T234406
Change-Id: Ie8ef047d2710c523b67bfc54fa2ad70fc47c5236
The main reasons for adding this service layer are:
* It allows error messages to be more consistent, by defining
a set of reportable information that can describe any block
type and is consistently formatted.
* It decouples formatting from the block classes, removing
their dependency on language, for the most part.
The service provides one public method, getMessage, which
returns a Message object whose key and parameters are
determined by the type of block. This should be used instead
of the deprecated AbstractBlock::getPermissionsError and
AbstractBlock::getBlockErrorParams.
Calls to AbstractBlock::getPermissionsError are replaced in
this patch.
Bug: T227174
Change-Id: I8caae7e30a46ef7120a86a4e5e6f30ae00855063
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
Set appropriate headers and flush the output as needed to avoid blocking
the client on post-send updates for the stock apache2 server scenario.
Several cases have bits of header logic to avoid delay:
a) basic GET/POST requests that succeed (e.g. HTTP 2XX)
b) requests that fail with errors (e.g. HTTP 500)
c) If-Modified-Since requests (e.g. HTTP 304)
d) HEAD requests
This last two still block on deferred updates, so schedulePostSendJobs()
does not trigger on them as a form of mitigation. Slow deferred updates
should only trigger on POST anyway (inline and redirect responses are
OK), so this should not be much of a problem.
Deprecate triggerJobs() and implement post-send job runs as a deferred.
This makes it easy to check for the existence of post-send updates by
calling DeferredUpdates::pendingUpdatesCount() after the pre-send stage.
Also, avoid running jobs on requests that had exceptions. Relatedly,
remove $mode option from restInPeace() and doPostOutputShutdown()
Only one caller was using the non-default options.
Bug: T206283
Change-Id: I2dd2b71f1ced0f4ef8b16ff41ffb23bb5b4c7028
The same way it does already for non-error output. This makes
it so that doPreOutputCommit() consistently happens between
the staging of output and the actual sending of output.
It is still allowed for code to bypass this, such as for fatal
errors and for handlers that disable OutputPage (like Special:Export).
But for cases where we do want to perform doPreOutputCommit(), it
should be run consistently between staging and sending so that it
can make appropiate decisions based on the current state of
OutputPage.
Previously, the state of OutputPage seen by doPreOutputCommit()
would be the broken/incomplete output of a seemingly succesful
(possibly cacheable) user action, which would then, after
doPreOutputCommit() runs, be completely replaced by $e->report()/
$out->showErrorPage().
This is a prerequisite for being able to reliably send cookie-block
cookies on error pages (next patch).
Bug: T233594
Change-Id: Iaeaf5e55a5868e6be534ddda73f3b56b9d6ef8f0
The variable is also read in a few other places, such as to
export the value from api.php (siteinfo) and load.php (mw.config)
but those requests don't need to be held back by this extra
logic.
Alternatively, if we really want to require this for all consumption,
we should probably let PathRouter provide the value and require
consumers to use it. E.g. services->getPathRouter->getArticlePath,
or something like that.
As easy first step, I'm moving it to PathRouter, called from
WebRequest::getPathInfo which is still called on all index.php
requests for any wiki page action in any namespace (incl Special)
when the wiki uses anything other than the default 'index.php?title='
article path.
Test Plan:
* Set '$wgArticlePath = 'bla';`
* View /mediawiki/index.php/Main_Page, and observe the fatal
error message (same as before this change).
Bug: T189966
Change-Id: Id06c2557e2addb58faeef0b6f7767a65b8de55a5
Follows-up d0439af89f.
If the UserNotLoggedIn class is constructed with an unsupported
message parameter, thrown, and handled by MWExceptionHandler, the
report() method would get called, and it would call the parent,
which stages a full error page and sends it via OutputPage::output.
Due to the missing return statement, it would then still execute
the remaining code, which messes up the internal state of the
already-sent OutputPage object by changing its redirect target
(which will never be used, but might confuse other consumers),
and trying to re-send output() and redirect headers, which will
fail with a warning.
Fixing this is required for T233594 and Iaeaf5e55a586, which allows
ErrorPageError to be "stage only" without ending output. Without
this fix, it would call the parent and do stage-only, but then
the remaining code in this method also work and actually succeed
at sending an invalid message to the user.
To preserve current (accidentally correct) behaviour, this needs
to be fixed first.
Bug: T233594
Bug: T17484
Change-Id: Ic5d73becd889839399a5b425cbbe22a3401acea9
These are reported by phan as PhanTypeMismatchArgumentNullableInternal
when null_casts_as_any_type is disabled.
Change-Id: I85076ee31c1bfc59a19600e84da0d915e425890a
This commit splits the existing Block class into AbstractBlock, Block
and SystemBlock.
Before this patch, the Block class represents several types of
blocks, which can be separated into blocks stored in the database,
and temporary blocks created by the system. These are now
represented by Block and SystemBlock, which inherit from
AbstractBlock.
This lays the foundations for:
* enforcing block parameters from multiple blocks that apply to a
user/IP address
* improvements to the Block API, including the addition of services
Breaking changes: functions expecting a Block object should still
expect a Block object if it came from the database, but other
functions may now need to expect an AbstractBlock or SystemBlock
object. (Note that an alternative naming scheme, in which the
abstract class is called Block and the subclasses are DatabaseBlock
and SystemBlock, avoids this breakage. However, it introduces more
breakages to calls to static Block methods and new Block
instantiations.)
Changes to tests: system blocks don't set the $blockCreateAccount or
$mExipry block properties, so remove/change any tests that assume
they do.
Bug: T222737
Change-Id: I83bceb5e5049e254c90ace060f8f8fad44696c67
