Remove calls to Str::contains and Arr::get when not necessary
#27
Conversation
| @@ -48,7 +48,7 @@ public function __construct(array $config) | |||
| $dsn = $this->getDsn($config); | |||
|
|
|||
| // You can pass options directly to the MongoDB constructor | |||
| $options = Arr::get($config, 'options', []); | |||
| $options = $config['options'] ?? []; | |||
There was a problem hiding this comment.
This is exactly what was done in Add::get. https://github.com/laravel/framework/blob/256f4974a09e24170ceeeb9e573651fd5e1c703e/src/Illuminate/Collections/Arr.php#L322-L324
There was a problem hiding this comment.
Noted that we're not using dot notation here (per Arr::get() docs), so these changes seem sensible.
| @@ -155,7 +155,7 @@ public function getAttribute($key) | |||
| } | |||
|
|
|||
| // Dot notation support. | |||
| if (Str::contains($key, '.') && Arr::has($this->attributes, $key)) { | |||
| if (str_contains($key, '.') && Arr::has($this->attributes, $key)) { | |||
There was a problem hiding this comment.
Based on https://laravel.com/docs/10.x/helpers#method-str-contains, am I correct to assume that Str::contains() only differs in its handling of array parameters?
Is it feasible to use scalar type hints for the getAttribute() signature, or would that conflict with inheritance? Likewise for other str_contains() changes below.
| @@ -48,7 +48,7 @@ public function __construct(array $config) | |||
| $dsn = $this->getDsn($config); | |||
|
|
|||
| // You can pass options directly to the MongoDB constructor | |||
| $options = Arr::get($config, 'options', []); | |||
| $options = $config['options'] ?? []; | |||
There was a problem hiding this comment.
Noted that we're not using dot notation here (per Arr::get() docs), so these changes seem sensible.
| @@ -195,7 +195,7 @@ public function setAttribute($key, $value) | |||
|
|
|||
| $value = $builder->convertKey($value); | |||
| } // Support keys in dot notation. | |||
| elseif (Str::contains($key, '.')) { | |||
| elseif (str_contains($key, '.')) { | |||
There was a problem hiding this comment.
Where does this package require PHP 8+? I don't see it in composer.json so I reckon it's an indirect dependency. I think it'd make sense to explicitly require it if we're going to be utilizing PHP 8+ functionality directly.
Hhappy to defer to a subsequent PHPORM ticket.
There was a problem hiding this comment.
It requires laravel/framework: ^10.0 which requires PHP 8.1+.
|
Moved to mongodb/laravel-mongodb#2571 |
No description provided.