Skip to content

fix: incorrect type declarations #5909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

fix: incorrect type declarations #5909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0a48e6a
fix: incorrect types in doc comments
kenjis Apr 17, 2022
df987a3
fix: incorrect type of `instanceof`
kenjis Apr 17, 2022
8a7113f
refactor: remove uneeded empty()
kenjis Apr 17, 2022
ac93fe4
fix: [BC] fix incorrect types
kenjis Apr 17, 2022
98f778e
docs: fix typos in doc comments
kenjis Apr 18, 2022
c470d25
docs: fix @return type
kenjis Apr 18, 2022
0ffc993
refactor: to avoid PHPStan error
kenjis Apr 18, 2022
fed3b85
fix: [BC] fix param types
kenjis Apr 18, 2022
03ba268
fix: add missing abstract methods
kenjis Apr 18, 2022
f89f42a
fix: CLIRequest in php-cli runs filters
kenjis Apr 20, 2022
ba5cf95
refactor: $request->getMethod() always returns string value
kenjis Jul 10, 2022
168b66c
chore: remove suppressed error messages
kenjis Jul 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 0 additions & 100 deletions phpstan-baseline.neon.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,36 +70,11 @@ parameters:
count: 1
path: system/Cache/Handlers/RedisHandler.php

-
message: "#^Call to an undefined method CodeIgniter\\\\HTTP\\\\Request\\:\\:getPost\\(\\)\\.$#"
count: 1
path: system/CodeIgniter.php

-
message: "#^Call to an undefined method CodeIgniter\\\\HTTP\\\\Request\\:\\:setLocale\\(\\)\\.$#"
count: 1
path: system/CodeIgniter.php

-
message: "#^Property CodeIgniter\\\\Database\\\\BaseBuilder\\:\\:\\$db \\(CodeIgniter\\\\Database\\\\BaseConnection\\) in empty\\(\\) is not falsy\\.$#"
count: 1
path: system/Database/BaseBuilder.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Database\\\\BaseConnection\\:\\:_disableForeignKeyChecks\\(\\)\\.$#"
count: 1
path: system/Database/BaseConnection.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Database\\\\BaseConnection\\:\\:_enableForeignKeyChecks\\(\\)\\.$#"
count: 1
path: system/Database/BaseConnection.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Database\\\\QueryInterface\\:\\:getOriginalQuery\\(\\)\\.$#"
count: 1
path: system/Database/BaseConnection.php

-
message: "#^Negated boolean expression is always false\\.$#"
count: 3
Expand All @@ -115,16 +90,6 @@ parameters:
count: 2
path: system/Database/BaseResult.php

-
message: "#^Access to an undefined property CodeIgniter\\\\Database\\\\ConnectionInterface\\:\\:\\$DBDriver\\.$#"
count: 2
path: system/Database/Database.php

-
message: "#^Access to an undefined property CodeIgniter\\\\Database\\\\ConnectionInterface\\:\\:\\$connID\\.$#"
count: 2
path: system/Database/Database.php

-
message: "#^Property CodeIgniter\\\\Database\\\\Migration\\:\\:\\$DBGroup \\(string\\) on left side of \\?\\? is not nullable\\.$#"
count: 1
Expand Down Expand Up @@ -400,26 +365,11 @@ parameters:
count: 1
path: system/Debug/Exceptions.php

-
message: "#^Call to an undefined method CodeIgniter\\\\View\\\\RendererInterface\\:\\:getPerformanceData\\(\\)\\.$#"
count: 1
path: system/Debug/Toolbar/Collectors/Events.php

-
message: "#^Property CodeIgniter\\\\Log\\\\Logger\\:\\:\\$logCache \\(array\\) on left side of \\?\\? is not nullable\\.$#"
count: 1
path: system/Debug/Toolbar/Collectors/Logs.php

-
message: "#^Call to an undefined method CodeIgniter\\\\View\\\\RendererInterface\\:\\:getData\\(\\)\\.$#"
count: 1
path: system/Debug/Toolbar/Collectors/Views.php

-
message: "#^Call to an undefined method CodeIgniter\\\\View\\\\RendererInterface\\:\\:getPerformanceData\\(\\)\\.$#"
count: 2
path: system/Debug/Toolbar/Collectors/Views.php

-
message: "#^Static property CodeIgniter\\\\Email\\\\Email\\:\\:\\$func_overload \\(bool\\) in isset\\(\\) is not nullable\\.$#"
count: 1
Expand Down Expand Up @@ -565,51 +515,6 @@ parameters:
count: 1
path: system/Images/Handlers/ImageMagickHandler.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Router\\\\RouteCollectionInterface\\:\\:getDefaultNamespace\\(\\)\\.$#"
count: 3
path: system/Router/Router.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Router\\\\RouteCollectionInterface\\:\\:getFilterForRoute\\(\\)\\.$#"
count: 1
path: system/Router/Router.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Router\\\\RouteCollectionInterface\\:\\:getFiltersForRoute\\(\\)\\.$#"
count: 1
path: system/Router/Router.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Router\\\\RouteCollectionInterface\\:\\:getRoutesOptions\\(\\)\\.$#"
count: 1
path: system/Router/Router.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Router\\\\RouteCollectionInterface\\:\\:isFiltered\\(\\)\\.$#"
count: 1
path: system/Router/Router.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Router\\\\RouteCollectionInterface\\:\\:setHTTPVerb\\(\\)\\.$#"
count: 1
path: system/Router/Router.php

-
message: "#^Call to an undefined method CodeIgniter\\\\Router\\\\RouteCollectionInterface\\:\\:getRegisteredControllers\\(.*\\)\\.$#"
count: 2
path: system/Router/Router.php

-
message: "#^Expression on left side of \\?\\? is not nullable\\.$#"
count: 1
path: system/Router/Router.php

-
message: "#^Method CodeIgniter\\\\Router\\\\RouteCollectionInterface\\:\\:getRoutes\\(\\) invoked with 1 parameter, 0 required\\.$#"
count: 1
path: system/Router/Router.php

-
message: "#^Strict comparison using \\=\\=\\= between string and true will always evaluate to false\\.$#"
count: 1
Expand Down Expand Up @@ -675,11 +580,6 @@ parameters:
count: 1
path: system/Validation/Validation.php

-
message: "#^Property CodeIgniter\\\\View\\\\Cell\\:\\:\\$cache \\(CodeIgniter\\\\Cache\\\\CacheInterface\\) in empty\\(\\) is not falsy\\.$#"
count: 2
path: system/View/Cell.php

-
message: "#^Property Config\\\\View\\:\\:\\$plugins \\(array\\) on left side of \\?\\? is not nullable\\.$#"
count: 1
Expand Down
11 changes: 8 additions & 3 deletions system/CodeIgniter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class CodeIgniter
/**
* Current request.
*
* @var CLIRequest|IncomingRequest|Request|null
* @var CLIRequest|IncomingRequest|null
*/
protected $request;

Expand Down Expand Up @@ -432,7 +432,10 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
return $returnResponse ? $possibleResponse : $possibleResponse->pretend($this->useSafeOutput)->send();
}

if ($possibleResponse instanceof Request) {
if (
$possibleResponse instanceof IncomingRequest
Copy link
Member

@lonnieezell lonnieezell Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think it unlikely, what if someone had created their own Request class? LIke perhaps a more PSR-friendly version. What is the benefit of this change other than limiting those options for devs?

|| $possibleResponse instanceof CLIRequest
) {
$this->request = $possibleResponse;
}

Expand Down Expand Up @@ -549,9 +552,11 @@ protected function startBenchmark()
* Sets a Request object to be used for this request.
* Used when running certain tests.
*
* @param CLIRequest|IncomingRequest $request
*
* @return $this
*/
public function setRequest(Request $request)
public function setRequest($request)
{
$this->request = $request;

Expand Down
3 changes: 1 addition & 2 deletions system/Config/Services.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
use CodeIgniter\Log\Logger;
use CodeIgniter\Pager\Pager;
use CodeIgniter\Router\RouteCollection;
use CodeIgniter\Router\RouteCollectionInterface;
use CodeIgniter\Router\Router;
use CodeIgniter\Security\Security;
use CodeIgniter\Session\Handlers\Database\MySQLiHandler;
Expand Down Expand Up @@ -597,7 +596,7 @@ public static function routes(bool $getShared = true)
*
* @return Router
*/
public static function router(?RouteCollectionInterface $routes = null, ?Request $request = null, bool $getShared = true)
public static function router(?RouteCollection $routes = null, ?Request $request = null, bool $getShared = true)
Copy link
Member

@lonnieezell lonnieezell Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it more difficult for people to pass their own classes that implement the RouteCollectionInterface, which they currently can, so this would be BC.

Copy link
Member Author

@kenjis kenjis Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current Router does not depend on RouteCollectionInterface, it actually depends on RouteCollection.

If a user creates their own RouteCollection which implements RouteCollectionInterface,
it is not compatible with the Router, because some methods are missing in the RouteCollectionInterface.

Copy link
Member

@MGatner MGatner Jul 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonnieezell can we pick this discussion back up?

Copy link
Member

@lonnieezell lonnieezell Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe change the Router class?

{
if ($getShared) {
return static::getSharedInstance('router', $routes, $request);
Expand Down
16 changes: 15 additions & 1 deletion system/Database/BaseConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,7 +914,7 @@ public function prepare(Closure $func, array $options = [])

$this->pretend(false);

if ($sql instanceof QueryInterface) {
if ($sql instanceof Query) {
Copy link
Member

@lonnieezell lonnieezell Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again - BC break since they can currently pass custom implementations.

Copy link
Member Author

@kenjis kenjis Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QueryInterface does not have getOriginalQuery().
So it can't be used.

Copy link
Member

@MGatner MGatner Jul 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonnieezell

Copy link
Member

@lonnieezell lonnieezell Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be less breaking to add getOriginalQuery to the interface? I realize they can just extend the original Query class to do custom implementations but that defeats the whole purpose of having an interface to begin with.

Without looking at the history of things I want to say that method came after the initial release, but even if it didn't I think the bug would be that it wasn't included.

$sql = $sql->getOriginalQuery();
}

Expand Down Expand Up @@ -1509,6 +1509,13 @@ public function disableForeignKeyChecks()
return $this->query($sql);
}

/**
* Returns platform-specific SQL to disable foreign key checks.
*
* @return string
*/
abstract protected function _disableForeignKeyChecks();

/**
* Enables foreign key checks temporarily.
*/
Expand All @@ -1519,6 +1526,13 @@ public function enableForeignKeyChecks()
return $this->query($sql);
}

/**
* Returns platform-specific SQL to enable foreign key checks.
*
* @return string
*/
abstract protected function _enableForeignKeyChecks();

/**
* Allows the engine to be set into a mode where queries are not
* actually executed, but they are still generated, timed, etc.
Expand Down
4 changes: 2 additions & 2 deletions system/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public function load(array $params = [], string $alias = '')
/**
* Creates a Forge instance for the current database type.
*/
public function loadForge(ConnectionInterface $db): object
public function loadForge(BaseConnection $db): object
{
if (! $db->connID) {
$db->initialize();
Expand All @@ -73,7 +73,7 @@ public function loadForge(ConnectionInterface $db): object
/**
* Creates a Utils instance for the current database type.
*/
public function loadUtils(ConnectionInterface $db): object
public function loadUtils(BaseConnection $db): object
{
if (! $db->connID) {
$db->initialize();
Expand Down
4 changes: 2 additions & 2 deletions system/Debug/Toolbar/Collectors/Events.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

namespace CodeIgniter\Debug\Toolbar\Collectors;

use CodeIgniter\View\RendererInterface;
use CodeIgniter\View\View;
use Config\Services;

/**
Expand Down Expand Up @@ -54,7 +54,7 @@ class Events extends BaseCollector
/**
* Instance of the Renderer service
*
* @var RendererInterface
* @var View
*/
protected $viewer;

Expand Down
4 changes: 2 additions & 2 deletions system/Debug/Toolbar/Collectors/Views.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

namespace CodeIgniter\Debug\Toolbar\Collectors;

use CodeIgniter\View\RendererInterface;
use CodeIgniter\View\View;
use Config\Services;

/**
Expand Down Expand Up @@ -62,7 +62,7 @@ class Views extends BaseCollector
/**
* Instance of the Renderer service
*
* @var RendererInterface
* @var View
*/
protected $viewer;

Expand Down
2 changes: 1 addition & 1 deletion system/HTTP/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public function getMethod(bool $upper = false): string
/**
* Sets the request method. Used when spoofing the request.
*
* @return Request
* @return $this
*
* @deprecated Use withMethod() instead for immutability
*
Expand Down
Loading