Fix stale/inaccurate PHPDoc in core/ (Auth, Cache, Columns, Common, Concurrency, Config)#24739
Open
sgiehl wants to merge 10 commits into
Open
Fix stale/inaccurate PHPDoc in core/ (Auth, Cache, Columns, Common, Concurrency, Config)#24739sgiehl wants to merge 10 commits into
sgiehl wants to merge 10 commits into
Conversation
BaseFactory is a generic base class meant to be extended by many different factory types (per its own class docblock), but its factory() method was documented as always returning a \Piwik\DataTable\Renderer, which only made sense for one specific subclass's usage.
setAcceptInvalidSSLCertificate()'s param had no type, and setConcurrentProcessesLimit()'s @param put the type after the variable name instead of before it. Added a type to Process::writePidFileContent()'s $content param.
getProcessId() can return false (from getmypid()) but was documented as returning int|null, a case that never actually occurs. sanitizeInputValue()/sanitizeLineBreaks() both explicitly handle a null input internally but were documented as accepting only string; widening resolves a previously-suppressed PHPStan finding. Added missing param types on convertUserIdToVisitorIdBin() and checkValidLanguagesIsSet().
Every method was fully untyped (@param with no type, @return mixed), even though the single implementation (MySqlLockBackend) and callers in Lock consistently use string keys/values, int TTLs, and bool/string return values.
The method takes no parameters, but its docblock still documented a $general param that no longer exists (the value is now read internally via $this->General). Also fixed the vague "mixed" return type to array.
This class is a file-based cache backend, not an exception; the docblock was copy-pasted from the neighboring ConfigNotFoundException class.
sgiehl
added
Technical debt
CI caught that widening this to `object` made PHPStan blind to a known, already-baselined type issue in ScheduledReports.php (a call to a Renderer-specific method), causing an "ignored error not matched" failure. BaseFactory has three subclasses (ReportRenderer, DataTable\Renderer, ImageGraph\StaticGraph) with different concrete return types that can't be expressed with a single annotation here, so restore the original, deliberately-tolerated `\Piwik\DataTable\Renderer` type rather than making the return type unusable for type-checking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of an ongoing pass to fix PHPDoc comments across
core/andplugins/(excluding@api-tagged classes/methods andAPI.phpclasses) that no longer match the actual code behavior, or whose@param/@returnannotations are wrong/incomplete.This PR covers:
core/BaseFactory.php:factory()was documented as always returning a\Piwik\DataTable\Renderer, but this is a generic base class meant to be extended by many different factory types.core/Cache.php: added a missing param type onbuildBackend().core/CliMulti.php/core/CliMulti/Process.php: fixed an untyped param, a misordered@param $limit int ...tag (type after the variable name), and added a missing type.core/Columns/DimensionsProvider.php: added a missing param type onfactory().core/Columns/Updater.php: added a missing@paramongetUpdatesForDimension().core/Common.php:getProcessId()can returnfalse(fromgetmypid()) but was documented asint|null(a case that never occurs);sanitizeInputValue()/sanitizeLineBreaks()both explicitly handle anullinput internally but were documented as accepting onlystring; added two missing param types.core/Concurrency/LockBackend.php: the entire interface was fully untyped (bare@param $x,@return mixed) despite the implementation and callers consistently using string/int/bool.core/Config.php:getDatatableRowLimits()takes no parameters, but its docblock still documented a$generalparam that no longer exists.core/Config/Cache.php: the class docblock said "Exception thrown when the config file doesn't exist" — copy-pasted from the neighboringConfigNotFoundExceptionclass, but this class is a file cache backend, not an exception.One
phpstan-baseline.neonentry was removed because theCommon.phpfix resolved a real, previously-suppressed PHPStan finding.Many files in this range were skipped because they are
@api-tagged (whole class or whole interface):core/Auth.php,core/Auth/Password.php,core/Auth/PasswordStrength.php,core/AuthResult.php,core/Category/Subcategory.php,core/Columns/ComputedMetricFactory.php,core/Columns/Dimension.php,core/Columns/DimensionMetricFactory.php,core/Columns/DimensionSegmentFactory.php,core/Columns/Discriminator.php,core/Columns/Join.php,core/Columns/Join/ActionNameJoin.php,core/Columns/Join/GoalNameJoin.php,core/Columns/Join/SiteNameJoin.php,core/Columns/MetricsList.php. Many other files needed no changes (already accurate/well-documented, or had no docblocks to begin with).Checklist