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

refactor: fix type inconsistency #11

Merged
merged 4 commits into from
Dec 17, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Dec 16, 2023

Needs #10

Description

  • fix type inconsistency

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis marked this pull request as draft December 16, 2023 02:03
@michalsn
Copy link
Member

Good catch. Thanks.

Comment on lines 96 to 97
$tries = (int) ($params['tries'] ?? CLI::getOption('tries'));
$retryAfter = (int) ($params['retry-after'] ?? CLI::getOption('retry-after'));
Copy link
Member

Choose a reason for hiding this comment

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

This has to be handled differently. We expect null if no value is set.

Copy link
Member Author

@kenjis kenjis Dec 17, 2023

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

And should be: 7cafc2d

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I made changes to support all the params: 440d844

@kenjis kenjis force-pushed the fix-type-inconsistency branch from a917ff0 to 7cafc2d Compare December 17, 2023 00:56
@kenjis
Copy link
Member Author

kenjis commented Dec 17, 2023

It seems the remaining errors are bugs or API inconsistency.

 ------ ----------------------------------------------------------------------------------------- 
  Line   src/Commands/QueueWork.php                                                               
 ------ ----------------------------------------------------------------------------------------- 
  195    Call to an undefined method CodeIgniter\Queue\Interfaces\JobInterface::getTries().       
  197    Call to an undefined method CodeIgniter\Queue\Interfaces\JobInterface::getRetryAfter().  
 ------ ----------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------- 
  Line   src/Handlers/BaseHandler.php                                                               
 ------ ------------------------------------------------------------------------------------------- 
  59     Call to an undefined method CodeIgniter\Queue\Handlers\BaseHandler::push().                
  72     Call to an undefined method CodeIgniter\Queue\Models\QueueJobFailedModel::affectedRows().  
  84     Call to an undefined method CodeIgniter\Queue\Models\QueueJobFailedModel::truncate().      
 ------ ------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------- 
  Line   src/Handlers/PredisHandler.php                                                           
 ------ ----------------------------------------------------------------------------------------- 
  97     Parameter #2 $fields of method Predis\ClientInterface::hdel() expects array, int given.  
  112    Parameter #2 $fields of method Predis\ClientInterface::hdel() expects array, int given.  
  125    Parameter #2 $fields of method Predis\ClientInterface::hdel() expects array, int given.  
 ------ ----------------------------------------------------------------------------------------- 

@kenjis kenjis force-pushed the fix-type-inconsistency branch from 7cafc2d to a2db811 Compare December 17, 2023 05:14
@michalsn
Copy link
Member

Thanks!

Seems like QueueJobFailedModel::affectedRows() and QueueJobFailedModel::truncate() are fine, the rest is fixed in #12.

@kenjis
Copy link
Member Author

kenjis commented Dec 17, 2023

I don't understand the code well, but is that model unrelated to Redis?

And you created #12, so can I close this PR?

@michalsn
Copy link
Member

I don't understand the code well, but is that model unrelated to Redis?

No. Failed jobs are always stored in the "traditional" database.

And you created #12, so can I close this PR?

Yes, however, please don't remove the branch yet - just in case.

@michalsn michalsn merged commit a2db811 into codeigniter4:develop Dec 17, 2023
9 of 11 checks passed
@kenjis kenjis deleted the fix-type-inconsistency branch December 17, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants