Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

individual checks before calling inject* methods #634

Merged
merged 2 commits into from
Aug 28, 2018
Merged

individual checks before calling inject* methods #634

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd91517
individual checks before calling inject* methods
pine3ree Aug 25, 2018
a193795
fix cs-check
pine3ree Aug 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/Container/ApplicationConfigInjectionDelegator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,13 @@ public function __invoke(ContainerInterface $container, string $serviceName, cal
}

$config = $container->get('config');
if (! isset($config['routes']) && ! isset($config['middleware_pipeline'])) {
return $application;
}

self::injectPipelineFromConfig($application, (array) $config);
self::injectRoutesFromConfig($application, (array) $config);
if (! empty($config['middleware_pipeline'])) {
self::injectPipelineFromConfig($application, (array) $config);
}
if (! empty($config['routes'])) {
self::injectRoutesFromConfig($application, (array) $config);
}

return $application;
}
Expand Down Expand Up @@ -172,7 +173,7 @@ public static function injectPipelineFromConfig(Application $application, array
*/
public static function injectRoutesFromConfig(Application $application, array $config) : void
{
if (! isset($config['routes']) || ! is_array($config['routes'])) {
if (empty($config['routes']) || ! is_array($config['routes'])) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this change. It's a public function and when used directly, you will get an undefined index notice.

Copy link
Contributor Author

@pine3ree pine3ree Aug 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hello @xtreamwayz , why?
empty acts like ! isset() plus not-empty value. I am just using a stronger checking. I just intercept empty arrays (or whatever) as well...

before: if not set or not array return (does not return early for empty arrays, no need to loop over an empty array)
my-mod: if not set or empty (whatever) or not array return

I am just making the function skip the for loop in case of an empty array

kind regards

Copy link
Member

Choose a reason for hiding this comment

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

Huh... for some reason I thought it would through a notification but it doesn't. Ignore my comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xtreamwayz ,no problem :-)
I was also thinking about storing the resulting locale in a request attribute instead of using Locale::setDefault() in order to make it compatible with async-php setups. What do you reckon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xtreamwayz , oh ...another thing I forgot... the $config var is casted to an array because the inject* methods requires it, so I left it alone...but since I now check for key existence $config should be an array or at least implement ArrayAccess. Usually if a config service exists in a zend-expressive application, it is an array, so we could also remove those argument castings unless we want to support a non array ArrayAccess configuration object.

kind regards

Copy link
Member

@geerteltink geerteltink Aug 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I was also thinking about storing the resulting locale in a request attribute instead of using Locale::setDefault() in order to make it compatible with async-php setups. What do you reckon?

This is what I do usually. So yes, go ahead.

EDIT: This is for the other PR right?

oh ...another thing I forgot... the $config var is casted to an array because the inject* methods requires it, so I left it alone...but since I now check for key existence $config should be an array or at least implement ArrayAccess. Usually if a config service exists in a zend-expressive application, it is an array, so we could also remove those argument castings unless we want to support a non array ArrayAccess configuration object.

Config should always be an array or ArrayAccess object for Aura.Di.

Usually what we do is $config = $container->get('config') ?? [];.

Copy link
Contributor Author

@pine3ree pine3ree Aug 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@xtreamwayz 1. Oh gosh...yes..right....i have a "few" repos open in so many browser tabs and too little sleep... 2. Good, I am glad I didn't remove the array castings since we need to support ArrayAccess for Aura.Di. best regards.

return;
}

Expand Down