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

Add return value to execute method #333

Merged
merged 3 commits into from
Mar 21, 2020
Merged

Add return value to execute method #333

merged 3 commits into from
Mar 21, 2020

Conversation

devonmather
Copy link
Contributor

@devonmather devonmather commented Mar 20, 2020

Fixes the following error as demonstrated with test in PR and error below. Occurs after updating to Laravel 7.x and Tenancy 2.3

TypeError: Return value of "Stancl\Tenancy\Tests\Etc\AddUserCommand::execute()" must be of the type int, NULL returned.

Let me know if further changes are needed ✌🏻

@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #333 into 2.x will increase coverage by 0.91%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x     #333      +/-   ##
============================================
+ Coverage     87.87%   88.78%   +0.91%     
  Complexity      431      431              
============================================
  Files            61       61              
  Lines          1212     1213       +1     
============================================
+ Hits           1065     1077      +12     
+ Misses          147      136      -11     
Impacted Files Coverage Δ Complexity Δ
src/Traits/TenantAwareCommand.php 77.77% <100.00%> (+77.77%) 3.00 <0.00> (ø)
src/Traits/HasATenantsOption.php 100.00% <0.00%> (+55.55%) 3.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30bab68...a10cfa8. Read the comment docs.

@stancl stancl added this to the 2.3.1 milestone Mar 21, 2020
@stancl
Copy link
Member

stancl commented Mar 21, 2020

Perfect quality PR, thank you so much! Will tag a release with this and one other bug fix tomorrow.

@stancl stancl merged commit cf339a5 into archtechx:2.x Mar 21, 2020
@@ -26,6 +26,8 @@ protected function execute(InputInterface $input, OutputInterface $output)
$this->laravel->call([$this, 'handle']);
});
}

return 1;
Copy link
Member

Choose a reason for hiding this comment

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

@devonmather Actually, why are we returning 1 here? What's the reason for this and why 1 and not 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is it must return an integer value, I think it's related to the latest symfony component for commands. Upon further reading just now. I misunderstood the error and figured a 0 would be if the command failed. But upon further reading it looks like the default return value is zero. I can created another PR this evening to fix.

Copy link
Member

Choose a reason for hiding this comment

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

@devonmather Seems like Laravel casts the return value from handle() into an int and returns that. See this: https://github.com/laravel/framework/blob/7.x/src/Illuminate/Console/Command.php#L134

I guess most handle() methods return nothing, so null, which is cast into 0 - as success? And then the handle() method can optionally return for example 1 as a specific exit code? I'll open a PR and ask you for review, if you'd be so kind. I don't have that much experience with commands.

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