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

Optimize: Services #3463

Merged
merged 6 commits into from
Aug 18, 2020
Merged

Optimize: Services #3463

merged 6 commits into from
Aug 18, 2020

Conversation

paulbalandan
Copy link
Member

Description
Remove unneeded lines of code by:

  • Switching to ?? where possible
  • No is_object checks for nullable typehints
  • Use Elvis operator where possible
  • et al.

I have a failing test for router though I didn't touch that before:

$routes  = $routes ?? static::routes();

// this line is missing in the original, causing the passed request object to be actually <null>,
// causing 'Undefined getMethod() on null' on CodeIgniter\Router\Router line 149.
// for fix, I added this line. but I don't know why Travis is not erring in here
$request = $request ?? static::request(); 

return new Router($routes, $request);

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Sorry, something went wrong.

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Changes for email() and encryption() are necessary but for the other cases... I'm not sure if we should go with BaseConfig or be more restrictive.

system/Config/Services.php Outdated Show resolved Hide resolved
system/Config/Services.php Outdated Show resolved Hide resolved
system/Config/Services.php Outdated Show resolved Hide resolved
system/Config/Services.php Outdated Show resolved Hide resolved
system/Config/Services.php Outdated Show resolved Hide resolved
system/Config/Services.php Outdated Show resolved Hide resolved
@paulbalandan
Copy link
Member Author

Personally, I prefer to use the more specific config class name as typehint instead of the generic BaseConfig. This way we can be sure that no one passes an instance of Config\Images for the Parser, for example. And I think no one ever did that.

@michalsn
Copy link
Member

michalsn commented Aug 9, 2020

Fair point, I think we can go with a specific class.

@paulbalandan
Copy link
Member Author

I think we should also consider the case of #3475 for services.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

If we can get all the type hints specified this looks good to me. I never knew that static protected was even an acceptable syntax! I also never knew ?: was called the "Elvis operator". TIL

@michalsn
Copy link
Member

Okay, it seems we all agree on the stricter types for $config.

@paulbalandan I looked at the Services file and we use BaseConfig as type in several places. Could you please review all services and fix types for $config?

@paulbalandan
Copy link
Member Author

I dont get how my changes affect Database Live tests.

@michalsn
Copy link
Member

We're supposed to use the currently loaded instance of the config file. We should use config('Validation') instead of new ValidationConfig. This applies to all services.

I'm also wondering shouldn't we use an explicit declaration for the config types, like \Config\Validation in the method declaration instead of ValidationConfig? Not because currently proposed version is wrong but I wonder if this won't raise questions among users who may not know what class this is? But maybe I'm exaggerating.

@paulbalandan
Copy link
Member Author

We're supposed to use the currently loaded instance of the config file. We should use config('Validation') instead of new ValidationConfig. This applies to all services.

Oh. I'll look into this.

I'm also wondering shouldn't we use an explicit declaration for the config types, like \Config\Validation in the method declaration instead of ValidationConfig? Not because currently proposed version is wrong but I wonder if this won't raise questions among users who may not know what class this is? But maybe I'm exaggerating.

I don't know but shouldn't this be already known if you're using PHP OOP? For me, I don't prefer using FQCN in typehints.

@MGatner
Copy link
Member

MGatner commented Aug 13, 2020

For me, I don't prefer using FQCN in typehints

I think this is probably a matter of opinion. Personally unless there is some compelling reason otherwise I will put use statements at the top of my file for every class in the file (including type hints) and then use the short name. This makes it easy to change classes later via aliasing and it acts as a reference for each file as to which classes are in use there.

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Just two tiny things.

system/Config/Services.php Outdated Show resolved Hide resolved
user_guide_src/source/changelogs/v4.0.5.rst Outdated Show resolved Hide resolved
@MGatner
Copy link
Member

MGatner commented Aug 16, 2020

This is looking good, and very close! Thanks you two.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Verified

This commit was signed with the committer’s verified signature.
paulbalandan John Paul E. Balandan, CPA

Verified

This commit was signed with the committer’s verified signature.
paulbalandan John Paul E. Balandan, CPA
@paulbalandan
Copy link
Member Author

I don't know what the phpstan error means. @samsonasik ?

@samsonasik
Copy link
Member

@paulbalandan you can remove // @phpstan-ignore-line in system/Config/Services.php line 358.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

@MGatner MGatner merged commit 6da204c into codeigniter4:develop Aug 18, 2020
@paulbalandan paulbalandan deleted the services-optimization branch August 18, 2020 20:14
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.

None yet

4 participants