Completed
Pull Request — master (#155)
by Rob
05:00 queued 02:39
created

Configuration::addSharedPluginNodes()   B

Complexity

Conditions 6
Paths 32

Size

Total Lines 91
Code Lines 74

Duplication

Lines 0
Ratio 0 %

Code Coverage

Tests 73
CRAP Score 6

Importance

Changes 0
Metric Value
dl 0
loc 91
ccs 73
cts 73
cp 1
rs 8.2585
c 0
b 0
f 0
cc 6
eloc 74
nc 32
nop 2
crap 6

How to fix   Long Method   

Long Method

Small methods make your code easier to understand, in particular if combined with a good name. Besides, if your method is small, finding a good name is usually much easier.

For example, if you find yourself adding comments to a method's body, this is usually a good sign to extract the commented part to a new method, and use the comment as a starting point when coming up with a good name for this new method.

Commonly applied refactorings include:

1
<?php
2
3
namespace Http\HttplugBundle\DependencyInjection;
4
5
use Http\Client\Common\Plugin\Cache\Generator\CacheKeyGenerator;
6
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
7
use Symfony\Component\Config\Definition\Builder\NodeDefinition;
8
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
9
use Symfony\Component\Config\Definition\ConfigurationInterface;
10
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
11
use Symfony\Component\Config\Definition\Exception\InvalidTypeException;
12
13
/**
14
 * This class contains the configuration information for the bundle.
15
 *
16
 * This information is solely responsible for how the different configuration
17
 * sections are normalized, and merged.
18
 *
19
 * @author David Buchmann <[email protected]>
20
 * @author Tobias Nyholm <[email protected]>
21
 */
22
class Configuration implements ConfigurationInterface
23
{
24
    /**
25
     * Whether to use the debug mode.
26
     *
27
     * @see https://github.com/doctrine/DoctrineBundle/blob/v1.5.2/DependencyInjection/Configuration.php#L31-L41
28
     *
29
     * @var bool
30
     */
31
    private $debug;
32
33
    /**
34
     * @param bool $debug
35
     */
36 19
    public function __construct($debug)
37
    {
38 19
        $this->debug = (bool) $debug;
39 19
    }
40
41
    /**
42
     * {@inheritdoc}
43
     */
44 19
    public function getConfigTreeBuilder()
45
    {
46 19
        $treeBuilder = new TreeBuilder();
47 19
        $rootNode = $treeBuilder->root('httplug');
48
49 19
        $this->configureClients($rootNode);
50 19
        $this->configureSharedPlugins($rootNode);
51
52
        $rootNode
1 ignored issue
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Symfony\Component\Config...\Builder\NodeDefinition as the method fixXmlConfig() does only exist in the following sub-classes of Symfony\Component\Config...\Builder\NodeDefinition: Symfony\Component\Config...der\ArrayNodeDefinition. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
53 19
            ->validate()
54
                ->ifTrue(function ($v) {
55 15
                    return !empty($v['classes']['client'])
56 15
                        || !empty($v['classes']['message_factory'])
57 12
                        || !empty($v['classes']['uri_factory'])
58 15
                        || !empty($v['classes']['stream_factory']);
59 19
                })
60
                ->then(function ($v) {
61 3
                    foreach ($v['classes'] as $key => $class) {
62 3
                        if (null !== $class && !class_exists($class)) {
63 1
                            throw new InvalidConfigurationException(sprintf(
64 1
                                'Class %s specified for httplug.classes.%s does not exist.',
65 1
                                $class,
66
                                $key
67 1
                            ));
68
                        }
69 2
                    }
70
71 2
                    return $v;
72 19
                })
73 19
            ->end()
74 19
            ->beforeNormalization()
75
                ->ifTrue(function ($v) {
76 19
                    return is_array($v) && array_key_exists('toolbar', $v) && is_array($v['toolbar']);
77 19
                })
78
                ->then(function ($v) {
79 4
                    if (array_key_exists('profiling', $v)) {
80 1
                        throw new InvalidConfigurationException('Can\'t configure both "toolbar" and "profiling" section. The "toolbar" config is deprecated as of version 1.3.0, please only use "profiling".');
81
                    }
82
83 3
                    @trigger_error('"httplug.toolbar" config is deprecated since version 1.3 and will be removed in 2.0. Use "httplug.profiling" instead.', E_USER_DEPRECATED);
1 ignored issue
show
Security Best Practice introduced by
It seems like you do not handle an error condition here. This can introduce security issues, and is generally not recommended.

If you suppress an error, we recommend checking for the error condition explicitly:

// For example instead of
@mkdir($dir);

// Better use
if (@mkdir($dir) === false) {
    throw new \RuntimeException('The directory '.$dir.' could not be created.');
}
Loading history...
84
85 3
                    if (array_key_exists('enabled', $v['toolbar']) && 'auto' === $v['toolbar']['enabled']) {
86 1
                        @trigger_error('"auto" value in "httplug.toolbar" config is deprecated since version 1.3 and will be removed in 2.0. Use a boolean value instead.', E_USER_DEPRECATED);
1 ignored issue
show
Security Best Practice introduced by
It seems like you do not handle an error condition here. This can introduce security issues, and is generally not recommended.

If you suppress an error, we recommend checking for the error condition explicitly:

// For example instead of
@mkdir($dir);

// Better use
if (@mkdir($dir) === false) {
    throw new \RuntimeException('The directory '.$dir.' could not be created.');
}
Loading history...
87 1
                        $v['toolbar']['enabled'] = $this->debug;
88 1
                    }
89
90 3
                    $v['profiling'] = $v['toolbar'];
91
92 3
                    unset($v['toolbar']);
93
94 3
                    return $v;
95 19
                })
96 19
            ->end()
97 19
            ->fixXmlConfig('client')
98 19
            ->children()
99 19
                ->arrayNode('main_alias')
100 19
                    ->addDefaultsIfNotSet()
101 19
                    ->info('Configure which service the main alias point to.')
102 19
                    ->children()
103 19
                        ->scalarNode('client')->defaultValue('httplug.client.default')->end()
104 19
                        ->scalarNode('message_factory')->defaultValue('httplug.message_factory.default')->end()
105 19
                        ->scalarNode('uri_factory')->defaultValue('httplug.uri_factory.default')->end()
106 19
                        ->scalarNode('stream_factory')->defaultValue('httplug.stream_factory.default')->end()
107 19
                    ->end()
108 19
                ->end()
109 19
                ->arrayNode('classes')
110 19
                    ->addDefaultsIfNotSet()
111 19
                    ->info('Overwrite a service class instead of using the discovery mechanism.')
112 19
                    ->children()
113 19
                        ->scalarNode('client')->defaultNull()->end()
114 19
                        ->scalarNode('message_factory')->defaultNull()->end()
115 19
                        ->scalarNode('uri_factory')->defaultNull()->end()
116 19
                        ->scalarNode('stream_factory')->defaultNull()->end()
117 19
                    ->end()
118 19
                ->end()
119 19
                ->arrayNode('profiling')
120 19
                    ->addDefaultsIfNotSet()
121 19
                    ->treatFalseLike(['enabled' => false])
122 19
                    ->treatTrueLike(['enabled' => true])
123 19
                    ->treatNullLike(['enabled' => $this->debug])
124 19
                    ->info('Extend the debug profiler with information about requests.')
125 19
                    ->children()
126 19
                        ->booleanNode('enabled')
127 19
                            ->info('Turn the toolbar on or off. Defaults to kernel debug mode.')
128 19
                            ->defaultValue($this->debug)
129 19
                        ->end()
130 19
                        ->scalarNode('formatter')->defaultNull()->end()
131 19
                        ->integerNode('captured_body_length')
132 19
                            ->defaultValue(0)
133 19
                            ->info('Limit long HTTP message bodies to x characters. If set to 0 we do not read the message body. Only available with the default formatter (FullHttpMessageFormatter).')
134 19
                        ->end()
135 19
                    ->end()
136 19
                ->end()
137 19
                ->arrayNode('discovery')
138 19
                    ->addDefaultsIfNotSet()
139 19
                    ->info('Control what clients should be found by the discovery.')
140 19
                    ->children()
141 19
                        ->scalarNode('client')
142 19
                            ->defaultValue('auto')
143 19
                            ->info('Set to "auto" to see auto discovered client in the web profiler. If provided a service id for a client then this client will be found by auto discovery.')
144 19
                        ->end()
145 19
                        ->scalarNode('async_client')
146 19
                            ->defaultNull()
147 19
                            ->info('Set to "auto" to see auto discovered client in the web profiler. If provided a service id for a client then this client will be found by auto discovery.')
148 19
                        ->end()
149 19
                    ->end()
150 19
                ->end()
151 19
            ->end();
152
153 19
        return $treeBuilder;
154
    }
155
156 19
    private function configureClients(ArrayNodeDefinition $root)
157
    {
158 19
        $root->children()
1 ignored issue
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Symfony\Component\Config...\Builder\NodeDefinition as the method fixXmlConfig() does only exist in the following sub-classes of Symfony\Component\Config...\Builder\NodeDefinition: Symfony\Component\Config...der\ArrayNodeDefinition. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
159 19
            ->arrayNode('clients')
160 19
                ->useAttributeAsKey('name')
161 19
                ->prototype('array')
162 19
                ->fixXmlConfig('plugin')
163 19
                ->validate()
164
                    ->ifTrue(function ($config) {
165
                        // Make sure we only allow one of these to be true
166 7
                        return (bool) $config['flexible_client'] + (bool) $config['http_methods_client'] + (bool) $config['batch_client'] >= 2;
167 19
                    })
168 19
                    ->thenInvalid('A http client can\'t be decorated with several of FlexibleHttpClient, HttpMethodsClient and BatchClient. Only one of the following options can be true. ("flexible_client", "http_methods_client", "batch_client")')
169 19
                ->end()
170 19
                ->children()
171 19
                    ->scalarNode('factory')
172 19
                        ->isRequired()
173 19
                        ->cannotBeEmpty()
174 19
                        ->info('The service id of a factory to use when creating the adapter.')
175 19
                    ->end()
176 19
                    ->booleanNode('flexible_client')
177 19
                        ->defaultFalse()
178 19
                        ->info('Set to true to get the client wrapped in a FlexibleHttpClient which emulates async or sync behavior.')
179 19
                    ->end()
180 19
                    ->booleanNode('http_methods_client')
181 19
                        ->defaultFalse()
182 19
                        ->info('Set to true to get the client wrapped in a HttpMethodsClient which emulates provides functions for HTTP verbs.')
183 19
                    ->end()
184 19
                    ->booleanNode('batch_client')
185 19
                        ->defaultFalse()
186 19
                        ->info('Set to true to get the client wrapped in a BatchClient which allows you to send multiple request at the same time.')
187 19
                    ->end()
188 19
                    ->variableNode('config')->defaultValue([])->end()
189 19
                    ->append($this->createClientPluginNode())
190 19
                ->end()
191 19
            ->end()
192 19
        ->end();
193 19
    }
194
195
    /**
196
     * @param ArrayNodeDefinition $root
197
     */
198 19
    private function configureSharedPlugins(ArrayNodeDefinition $root)
199
    {
200
        $pluginsNode = $root
201 19
            ->children()
202 19
                ->arrayNode('plugins')
203 19
                ->info('Global plugin configuration. Plugins need to be explicitly added to clients.')
204 19
                ->addDefaultsIfNotSet()
205
            // don't call end to get the plugins node
206 19
        ;
207 19
        $this->addSharedPluginNodes($pluginsNode);
208 19
    }
209
210
    /**
211
     * Createplugins node of a client.
212
     *
213
     * @return ArrayNodeDefinition The plugin node
214
     */
215 19
    private function createClientPluginNode()
216
    {
217 19
        $builder = new TreeBuilder();
218 19
        $node = $builder->root('plugins');
219
220
        /** @var ArrayNodeDefinition $pluginList */
221
        $pluginList = $node
222 19
            ->info('A list of plugin service ids and client specific plugin definitions. The order is important.')
223 19
            ->prototype('array')
224 19
        ;
225
        $pluginList
226
            // support having just a service id in the list
227 19
            ->beforeNormalization()
228
                ->always(function ($plugin) {
229 8
                    if (is_string($plugin)) {
230
                        return [
231
                            'reference' => [
232 7
                                'enabled' => true,
233 7
                                'id' => $plugin,
234 7
                            ],
235 7
                        ];
236
                    }
237
238 5
                    return $plugin;
239 19
                })
240 19
            ->end()
241
242 19
            ->validate()
243
                ->always(function ($plugins) {
244 7
                    foreach ($plugins as $name => $definition) {
245 7
                        if ('authentication' === $name) {
246 7
                            if (!count($definition)) {
247 7
                                unset($plugins['authentication']);
248 7
                            }
249 7
                        } elseif (!$definition['enabled']) {
250 7
                            unset($plugins[$name]);
251 7
                        }
252 7
                    }
253
254 7
                    return $plugins;
255 19
                })
256 19
            ->end()
257
        ;
258 19
        $this->addSharedPluginNodes($pluginList, true);
259
260
        $pluginList
261 19
            ->children()
262 19
                ->arrayNode('reference')
263 19
                    ->canBeEnabled()
264 19
                    ->info('Reference to a plugin service')
265 19
                    ->children()
266 19
                        ->scalarNode('id')
267 19
                            ->info('Service id of a plugin')
268 19
                            ->isRequired()
269 19
                            ->cannotBeEmpty()
270 19
                        ->end()
271 19
                    ->end()
272 19
                ->end()
273 19
                ->arrayNode('add_host')
274 19
                    ->canBeEnabled()
275 19
                    ->addDefaultsIfNotSet()
276 19
                    ->info('Set scheme, host and port in the request URI.')
277 19
                    ->children()
278 19
                        ->scalarNode('host')
279 19
                            ->info('Host name including protocol and optionally the port number, e.g. https://api.local:8000')
280 19
                            ->isRequired()
281 19
                            ->cannotBeEmpty()
282 19
                        ->end()
283 19
                        ->scalarNode('replace')
284 19
                            ->info('Whether to replace the host if request already specifies one')
285 19
                            ->defaultValue(false)
286 19
                        ->end()
287 19
                    ->end()
288 19
                ->end()
289 19
                ->arrayNode('header_append')
290 19
                    ->canBeEnabled()
291 19
                    ->info('Append headers to the request. If the header already exists the value will be appended to the current value.')
292 19
                    ->fixXmlConfig('header')
293 19
                    ->children()
294 19
                        ->arrayNode('headers')
295 19
                            ->info('Keys are the header names, values the header values')
296 19
                            ->normalizeKeys(false)
297 19
                            ->useAttributeAsKey('name')
298 19
                            ->prototype('scalar')->end()
299 19
                        ->end()
300 19
                    ->end()
301 19
                ->end()
302 19
                ->arrayNode('header_defaults')
303 19
                    ->canBeEnabled()
304 19
                    ->info('Set header to default value if it does not exist.')
305 19
                    ->fixXmlConfig('header')
306 19
                    ->children()
307 19
                        ->arrayNode('headers')
308 19
                            ->info('Keys are the header names, values the header values')
309 19
                            ->normalizeKeys(false)
310 19
                            ->useAttributeAsKey('name')
311 19
                            ->prototype('scalar')->end()
312 19
                        ->end()
313 19
                    ->end()
314 19
                ->end()
315 19
                ->arrayNode('header_set')
316 19
                    ->canBeEnabled()
317 19
                    ->info('Set headers to requests. If the header does not exist it wil be set, if the header already exists it will be replaced.')
318 19
                    ->fixXmlConfig('header')
319 19
                    ->children()
320 19
                        ->arrayNode('headers')
321 19
                            ->info('Keys are the header names, values the header values')
322 19
                            ->normalizeKeys(false)
323 19
                            ->useAttributeAsKey('name')
324 19
                            ->prototype('scalar')->end()
325 19
                        ->end()
326 19
                    ->end()
327 19
                ->end()
328 19
                ->arrayNode('header_remove')
329 19
                    ->canBeEnabled()
330 19
                    ->info('Remove headers from requests.')
331 19
                    ->fixXmlConfig('header')
332 19
                    ->children()
333 19
                        ->arrayNode('headers')
334 19
                            ->info('List of header names to remove')
335 19
                            ->prototype('scalar')->end()
336 19
                        ->end()
337 19
                    ->end()
338 19
                ->end()
339 19
            ->end()
340 19
        ->end();
341
342 19
        return $node;
343
    }
344
345
    /**
346
     * Add the definitions for shared plugin configurations.
347
     *
348
     * @param ArrayNodeDefinition $pluginNode The node to add to.
349
     * @param bool                $disableAll Some shared plugins are enabled by default. On the client, all are disabled by default.
350
     */
351 19
    private function addSharedPluginNodes(ArrayNodeDefinition $pluginNode, $disableAll = false)
352
    {
353 19
        $children = $pluginNode->children();
354
355 19
        $children->append($this->createAuthenticationPluginNode());
356 19
        $children->append($this->createCachePluginNode());
357
358
        $children
359 19
            ->arrayNode('cookie')
360 19
                ->canBeEnabled()
361 19
                ->children()
362 19
                    ->scalarNode('cookie_jar')
363 19
                        ->info('This must be a service id to a service implementing Http\Message\CookieJar')
364 19
                        ->isRequired()
365 19
                        ->cannotBeEmpty()
366 19
                    ->end()
367 19
                ->end()
368 19
            ->end();
369
        // End cookie plugin
370
371
        $children
372 19
            ->arrayNode('history')
373 19
                ->canBeEnabled()
374 19
                ->children()
375 19
                    ->scalarNode('journal')
376 19
                        ->info('This must be a service id to a service implementing Http\Client\Plugin\Journal')
377 19
                        ->isRequired()
378 19
                        ->cannotBeEmpty()
379 19
                    ->end()
380 19
                ->end()
381 19
            ->end();
382
        // End history plugin
383
384 19
        $decoder = $children->arrayNode('decoder');
385 19
        $disableAll ? $decoder->canBeEnabled() : $decoder->canBeDisabled();
386 19
        $decoder->addDefaultsIfNotSet()
387 19
            ->children()
388 19
                ->scalarNode('use_content_encoding')->defaultTrue()->end()
389 19
            ->end()
390 19
        ->end();
391
        // End decoder plugin
392
393 19
        $logger = $children->arrayNode('logger');
394 19
        $disableAll ? $logger->canBeEnabled() : $logger->canBeDisabled();
395 19
        $logger->addDefaultsIfNotSet()
396 19
            ->children()
397 19
                ->scalarNode('logger')
398 19
                    ->info('This must be a service id to a service implementing Psr\Log\LoggerInterface')
399 19
                    ->defaultValue('logger')
400 19
                    ->cannotBeEmpty()
401 19
                ->end()
402 19
                ->scalarNode('formatter')
403 19
                    ->info('This must be a service id to a service implementing Http\Message\Formatter')
404 19
                    ->defaultNull()
405 19
                ->end()
406 19
            ->end()
407 19
        ->end();
408
        // End logger plugin
409
410 19
        $redirect = $children->arrayNode('redirect');
411 19
        $disableAll ? $redirect->canBeEnabled() : $redirect->canBeDisabled();
412 19
        $redirect->addDefaultsIfNotSet()
413 19
            ->children()
414 19
                ->scalarNode('preserve_header')->defaultTrue()->end()
415 19
                ->scalarNode('use_default_for_multiple')->defaultTrue()->end()
416 19
            ->end()
417 19
        ->end();
418
        // End redirect plugin
419
420 19
        $retry = $children->arrayNode('retry');
421 19
        $disableAll ? $retry->canBeEnabled() : $retry->canBeDisabled();
422 19
        $retry->addDefaultsIfNotSet()
423 19
            ->children()
424 19
                ->scalarNode('retry')->defaultValue(1)->end() // TODO: should be called retries for consistency with the class
425 19
            ->end()
426 19
        ->end();
427
        // End retry plugin
428
429 19
        $stopwatch = $children->arrayNode('stopwatch');
430 19
        $disableAll ? $stopwatch->canBeEnabled() : $stopwatch->canBeDisabled();
431 19
        $stopwatch->addDefaultsIfNotSet()
432 19
            ->children()
433 19
                ->scalarNode('stopwatch')
434 19
                    ->info('This must be a service id to a service extending Symfony\Component\Stopwatch\Stopwatch')
435 19
                    ->defaultValue('debug.stopwatch')
436 19
                    ->cannotBeEmpty()
437 19
                ->end()
438 19
            ->end()
439 19
        ->end();
440
        // End stopwatch plugin
441 19
    }
442
443
    /**
444
     * Create configuration for authentication plugin.
445
     *
446
     * @return NodeDefinition Definition for the authentication node in the plugins list.
447
     */
448 19
    private function createAuthenticationPluginNode()
449
    {
450 19
        $builder = new TreeBuilder();
451 19
        $node = $builder->root('authentication');
452
        $node
1 ignored issue
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Symfony\Component\Config...\Builder\NodeDefinition as the method children() does only exist in the following sub-classes of Symfony\Component\Config...\Builder\NodeDefinition: Symfony\Component\Config...der\ArrayNodeDefinition. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
453 19
            ->useAttributeAsKey('name')
454 19
            ->prototype('array')
455 19
                ->validate()
456 19
                    ->always()
457
                    ->then(function ($config) {
458 5
                        switch ($config['type']) {
459 5
                            case 'basic':
460 4
                                $this->validateAuthenticationType(['username', 'password'], $config, 'basic');
461 4
                                break;
462 2
                            case 'bearer':
463 1
                                $this->validateAuthenticationType(['token'], $config, 'bearer');
464 1
                                break;
465 2
                            case 'service':
466 2
                                $this->validateAuthenticationType(['service'], $config, 'service');
467 1
                                break;
468 1
                            case 'wsse':
469 1
                                $this->validateAuthenticationType(['username', 'password'], $config, 'wsse');
470 1
                                break;
471 4
                        }
472
473 4
                        return $config;
474 19
                    })
475 19
                ->end()
476 19
                ->children()
477 19
                    ->enumNode('type')
478 19
                        ->values(['basic', 'bearer', 'wsse', 'service'])
479 19
                        ->isRequired()
480 19
                        ->cannotBeEmpty()
481 19
                    ->end()
482 19
                    ->scalarNode('username')->end()
483 19
                    ->scalarNode('password')->end()
484 19
                    ->scalarNode('token')->end()
485 19
                    ->scalarNode('service')->end()
486 19
                    ->end()
487 19
                ->end()
488 19
            ->end(); // End authentication plugin
489
490 19
        return $node;
491
    }
492
493
    /**
494
     * Validate that the configuration fragment has the specified keys and none other.
495
     *
496
     * @param array  $expected Fields that must exist
497
     * @param array  $actual   Actual configuration hashmap
498
     * @param string $authName Name of authentication method for error messages
499
     *
500
     * @throws InvalidConfigurationException If $actual does not have exactly the keys specified in $expected (plus 'type')
501
     */
502 5
    private function validateAuthenticationType(array $expected, array $actual, $authName)
503
    {
504 5
        unset($actual['type']);
505 5
        $actual = array_keys($actual);
506 5
        sort($actual);
507 5
        sort($expected);
508
509 5
        if ($expected === $actual) {
510 4
            return;
511
        }
512
513 1
        throw new InvalidConfigurationException(sprintf(
514 1
            'Authentication "%s" requires %s but got %s',
515 1
            $authName,
516 1
            implode(', ', $expected),
517 1
            implode(', ', $actual)
518 1
        ));
519
    }
520
521
    /**
522
     * Create configuration for cache plugin.
523
     *
524
     * @return NodeDefinition Definition for the cache node in the plugins list.
525
     */
526 19
    private function createCachePluginNode()
527
    {
528 19
        $builder = new TreeBuilder();
529
530 19
        $config = $builder->root('config');
531
        $config
1 ignored issue
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Symfony\Component\Config...\Builder\NodeDefinition as the method children() does only exist in the following sub-classes of Symfony\Component\Config...\Builder\NodeDefinition: Symfony\Component\Config...der\ArrayNodeDefinition. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
532 19
            ->fixXmlConfig('method')
533 19
            ->fixXmlConfig('respect_response_cache_directive')
534 19
            ->addDefaultsIfNotSet()
535 19
            ->validate()
536 19
                ->ifTrue(function ($config) {
537
                    // Cannot set both respect_cache_headers and respect_response_cache_directives
538 3
                    return isset($config['respect_cache_headers'], $config['respect_response_cache_directives']);
539 19
                })
540 19
                ->thenInvalid('You can\'t provide config option "respect_cache_headers" and "respect_response_cache_directives" simultaniously. Use "respect_response_cache_directives" instead.')
541 19
            ->end()
542 19
            ->children()
543 19
                ->scalarNode('cache_key_generator')
544
                    ->info('A class implementing '.CacheKeyGenerator::class.' to generate a PSR-6 cache key')
545
                ->end()
546
                ->integerNode('cache_lifetime')
547
                    ->info('The minimum time we should store a cache item')
548
                ->end()
549
                ->integerNode('default_ttl')
550
                    ->info('The default max age of a Response')
551
                ->end()
552
                ->enumNode('hash_algo')
553
                    ->info('Hashing algorithm to use')
554
                    ->values(hash_algos())
555
                    ->cannotBeEmpty()
556
                ->end()
557
                ->arrayNode('methods')
558
                    ->info('Which request methods to cache')
559
                    ->defaultValue(['GET', 'HEAD'])
560
                    ->prototype('scalar')
561
                        ->validate()
562 19
                            ->ifTrue(function ($v) {
563
                                /* RFC7230 sections 3.1.1 and 3.2.6 except limited to uppercase characters. */
564 1
                                return preg_match('/[^A-Z0-9!#$%&\'*+\-.^_`|~]+/', $v);
565 19
                            })
566 19
                            ->thenInvalid('Invalid method: %s')
567 19
                        ->end()
568 19
                    ->end()
569 19
                ->end()
570 19
                ->enumNode('respect_cache_headers')
571 19
                    ->info('Whether we should care about cache headers or not [DEPRECATED]')
572 19
                    ->values([null, true, false])
573 19
                    ->beforeNormalization()
574
                        ->always(function ($v) {
575 2
                            @trigger_error('The option "respect_cache_headers" is deprecated since version 1.3 and will be removed in 2.0. Use "respect_response_cache_directives" instead.', E_USER_DEPRECATED);
1 ignored issue
show
Security Best Practice introduced by
It seems like you do not handle an error condition here. This can introduce security issues, and is generally not recommended.

If you suppress an error, we recommend checking for the error condition explicitly:

// For example instead of
@mkdir($dir);

// Better use
if (@mkdir($dir) === false) {
    throw new \RuntimeException('The directory '.$dir.' could not be created.');
}
Loading history...
576
577 2
                            return $v;
578 19
                        })
579 19
                    ->end()
580 19
                ->end()
581 19
                ->variableNode('respect_response_cache_directives')
582 19
                    ->info('A list of cache directives to respect when caching responses')
583 19
                    ->validate()
584
                    ->always(function ($v) {
585 2
                        if (is_null($v) || is_array($v)) {
586 2
                            return $v;
587
                        }
588
589
                        throw new InvalidTypeException();
590 19
                    })
591 19
                    ->end()
592 19
                    ->end()
593 19
                ->end()
594 19
            ->end()
595
        ;
596
597 19
        $cache = $builder->root('cache');
598
        $cache
599 19
            ->canBeEnabled()
600 19
            ->addDefaultsIfNotSet()
601 19
            ->children()
602 19
                ->scalarNode('cache_pool')
603 19
                    ->info('This must be a service id to a service implementing Psr\Cache\CacheItemPoolInterface')
604 19
                    ->isRequired()
605 19
                    ->cannotBeEmpty()
606 19
                ->end()
607 19
                ->scalarNode('stream_factory')
608 19
                    ->info('This must be a service id to a service implementing Http\Message\StreamFactory')
609 19
                    ->defaultValue('httplug.stream_factory')
610 19
                    ->cannotBeEmpty()
611 19
                ->end()
612 19
            ->end()
613 19
            ->append($config)
614
        ;
615
616 19
        return $cache;
617
    }
618
}
619