Skip to content
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.

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

fix deprecations #326

Merged
merged 9 commits into from
May 23, 2022
Merged

Conversation

LordSimal
Copy link
Contributor

@LordSimal LordSimal commented Apr 20, 2022

Fixes #325

BUT there is still a problem which I can't really get my head around.

Its about the new ->fetchTable() method from the LocatorAwareTrait not returning the "correct" types even though https://github.com/CakeDC/cakephp-phpstan is installed.

I was able to easily replace the deprecated Cake\Filesystem\Folder class usages with just some plain old glob() functions. No need for SPL classes.

The changes in the composer.json are just my editor re-ordering the packages alphabatically. If this is a problem I can for sure just return that to its original state (and I declared to use phpstan 1.5 and requiring PHP 7.4)

@LordSimal
Copy link
Contributor Author

Had to bump up to "cakephp/cakephp": "^4.3.0", otherwise prefer-lowest wouldn't work because fetchTable() has been introduced in 4.3

@LordSimal
Copy link
Contributor Author

But I still have no idea how we should handle the missing "correct" return-types of fetchTable()

@@ -79,7 +79,7 @@ public function execute(Arguments $args, ConsoleIo $io) {
$io->out();

$io->out('Total unfinished jobs: ' . $this->QueuedJobs->getLength());
$this->loadModel('Queue.QueueProcesses');
$this->QueueProcesses = $this->fetchTable('Queue.QueueProcesses');
Copy link
Owner

Choose a reason for hiding this comment

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

something must still be wrong, as PHPStan doesnt like it yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now replaced fetchTable() with getTableLocator()->get($tableName) since this for some reason pleases the PHPStan gods...


$taskKey = $plugin ? $plugin . '.' . $name : $name;
$tasks[$taskKey] = $path . $r;
if ($res) {
Copy link
Owner

Choose a reason for hiding this comment

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

instead of nesting, why not just glob() ?: [] ?

Copy link
Contributor Author

@LordSimal LordSimal May 12, 2022

Choose a reason for hiding this comment

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

Sorry, I am not really sure what you mean. How would glob() ?: [] help us here?
We still need the separate $tasks variable since we have to return some sort of array so this seems pretty fine to me.

Copy link
Owner

Choose a reason for hiding this comment

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

The if nesting seems weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I get it now

if (!in_array($className, $ignoredTasks, true)) {
$tasks[$key] = $className;
$files = glob($path . '*Task.php');
if ($files) {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here

@@ -95,9 +95,10 @@ public function __construct(?Io $io = null, ?LoggerInterface $logger = null) {
$this->io = $io ?: new Io(new ConsoleIo());
$this->logger = $logger;

$this->loadModel($this->queueModelClass);
$tableLocator = $this->getTableLocator();
$this->QueuedJobs = $tableLocator->get($this->queueModelClass);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure how we should tackle this. Should we remove that property, set it hardcoded as string and therefore remove the posibility to set a custom QueueModelClass?

Copy link

@mainpal1 mainpal1 May 22, 2022

Choose a reason for hiding this comment

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

Could you please try the following? Not really sure if it would work.

        /**
         * @var \Queue\Model\Table\QueuedJobsTable
         */
        $QueuedJobs = $tableLocator->get($this->queueModelClass);

        $this->QueuedJobs = $QueuedJobs;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks pretty good! Thanks 👍🏻

@LordSimal
Copy link
Contributor Author

@dereuromark please check again if everything seems OK now.

@dereuromark dereuromark merged commit 8a4d71e into dereuromark:master May 23, 2022
@LordSimal LordSimal deleted the fix-deprecations branch May 23, 2022 10:56
@LordSimal LordSimal mentioned this pull request Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecated methods and classes
3 participants