Insphpect

This tool is currently proof-of-concept. Your feedback and evaluation is valuable in helping to improve it and ensure its reports are meaninful.

Please click here to complete a short survey to tell us what you think. It should take less than 5 minutes and help further this research project!

Symfony\Component\Routing\Loader\YamlFileLoader

Detected issues

Issue Method Line number
Global/Static variables NA 34

Code

Click highlighted lines for details

<?php/* * This file is part of the Symfony package. * * (c) Fabien Potencier <fabien@symfony.com> * * For the full copyright and license information, please view the LICENSE * file that was distributed with this source code. */namespace Symfony\Component\Routing\Loader;use Symfony\Component\Config\Loader\FileLoader;use Symfony\Component\Config\Resource\FileResource;use Symfony\Component\Routing\Loader\Configurator\Traits\LocalizedRouteTrait;use Symfony\Component\Routing\Loader\Configurator\Traits\PrefixTrait;use Symfony\Component\Routing\RouteCollection;use Symfony\Component\Yaml\Exception\ParseException;use Symfony\Component\Yaml\Parser as YamlParser;use Symfony\Component\Yaml\Yaml;/** * YamlFileLoader loads Yaml routing files. * * @author Fabien Potencier <fabien@symfony.com> * @author Tobias Schultze <http://tobion.de> */class YamlFileLoader extends FileLoader{    use LocalizedRouteTrait;    use PrefixTrait;

Global variables

Note: A future update will differentiate between private static variables and public static or global variables as private static variables do not cause as much of a problem.

Summary

  • Hidden dependencies
  • Broken encapsulation
  • One component can accidentally overwrite data required by another component (action at a distance)
  • You can only every have one copy of the variable
  • Adding code requires knowing exactly what variables are already in use
  • When working in teams, name clashes can be easily introduced
  • Global state makes it difficult to reuse the code. E.g. having two files open at the same time would require writing the code twice, three times for three files, etc.

Background

The identification of global variables as a bad practice dates as far back at least as far back as 1973[1] and are one of the most widespread and well known bad practices related to flexibility. This is likely due to being available in almost every programming language, ease of use and speed to learn. They also cause severe problems in code and it's very easy to get caught out by using them, even in a small application.

Global vairables are widely labelled "bad practice" and have been for some time, for example back in 1999 Kernighan wrote:

Avoid global variables; wherever possible it is better to pass references to all data through function arguments

Kernighan[2]

And Hevery[3] states:

I hope that by now most developers agree that global state should be treated like GOTO.

This attitude is widespread and Sayfan[4] sums up the problem:

Whenever shared mutable state is involved, it is easy for components to step on each other's toes.

Although "global variables are bad" is a common thing to here, for novice developers it's not immediately obvious why this is. However, the reasons have been covered frequently by developers of varying prominence. While writing about desiging the Eiffel programming language, [5] stated several problems with global variables:

Since global variables are shared by different modules, they make each of these modules more difficult to understand separately, diminishing readability and hence hampering maintenance.

As global variables constitute a form of undercover dependency between modules, they are a major obstacle to software evolution, since they make it harder to modify a module without impacting others.

They are a major source of nasty errors. Through a global variable, an error in a module may propagate to many others. As a result, the manifestation of the error may be quite remote from its cause in the software architecture, making it very hard to trace down errors and correct them. This problem is particularly serious in environments where incorrect array references may pollute other data.

Action at a distance

This problem is commonly referred to as action at a distance and described by Hevery[6] as:

Spooky Action at a Distance is when we run one thing that we believe is isolated (since we did not pass any references in) but unexpected interactions and state changes happen in distant locations of the system which we did not tell the object about. This can only happen via global state.

Broken encapsulation

The biggest issue with global variables (even private static variables) is that they break encapsulation. A class no longer has its own state and one instance of a class can affect the state of another. Although private static variables are the by far the least worst type of global variables they should still be refactored out where possible.

> Encapsulation refers to the bundling of data with the methods that operate on that data

By making the data globally accessible, encapsulation has been lost. Any part of the program has access to the data and can modify it. Even when using private static variables, each instance no longer has control of its own sate.

Tight coupling

Global variables introduce tight coupling. In Object Oriented Programming an object should be self-contained[7][8]. If a class depends on a global (or static) variable, then moving the class to a different project requires defining the required global variables in the new project. Private static variables do not not introduce additional coupling.

Name Clashes

Global variables can introduce name clashes:

everywhere in the program, you would have to keep track of the names of all the variables declared anywhere else in the program, so that you didn't accidentally re-use one.

Summit[9]

The problem of name clashes is magnified by the size of a team. If two people are working on a piece of software and both use global variables, it's possible they'll write some code using the same variable names. During execution this might cause the two peices of code to interfere with each other.

Examples

As a very crude example, imagine the following:

 function getUser($id) {
    
$connection Database::$connection;

    
$connection->query('SELET * FROM user ...')
}

This code assumes that Database::$connection has been set correctly and not overwritten. If any part of the application accidently runs the code Database::$connection = null; (or sets it to anything other than a database connection) then the code will fail. This is due to broken encapsulation and action and a distance.

Anything in the code is able to change the property and cause unexpected behaviour when further methods are called on the instance.

This can also happen with private static properties:

 
class FileReadWrite {
    private static 
$fileName;

    public function 
__construct(string $file) {
        
self::$fileName $file;
    }

    public function 
read() {
        return 
file_get_contents(self::$fileName);
    }

    public function 
write(string $data) {
        
file_put_contents(self::$fileName$data);
    }
}

 
//Works as expected:

$file = new FileReadWrite('./one.txt');
$file->write('data');


//cause a problem

$file1 = new FileReadWrite('./one.txt');
$file2 = new FileReadWrite('./two.txt');

$file1->write('data1');
$file2->write('data2');

This causes a problem because there is a global variable storing the file name. Assuming a class requires only one value of a variable across the whole application always limits flexibility. There are occasionally practical reasons for this such as keeping track of and limiting the number of open files/connections but flexibility is always reduced. Even in these practical exceptions, it introduces a new issue of separation of concerns: Should the class be concerned with the number of open connections throughout the application or should that be managed at an application level rather than a class level?

Although this is a contrived example, the same kind of bugs can occur any time a static variable is used. If it's set by one instance and read by another then unexpected changes can cause bugs.

Further reading

References

  1. Wulf, W., Shaw, M. (1973) Global varaibles considered harmful. ACM SIGPLAN Notices , pp.28-34.
  2. Kernighan, B. (1999) The Practice of Programming ISBN: 978-0201615869. Addison Wesley.
  3. Hevery, M. (2008) Top 10 things which make your code hard to test [online]. Available from: http://misko.hevery.com/2008/07/30/top-10-things-which-make-your-code-hard-to-test/
  4. Sayfan, M. (n.d.) Avoid Global Variables, Environment Variables, and Singletons [online]. Available from: https://sites.google.com/site/michaelsafyan/software-engineering/avoid-global-variables-environment-variables-and-singletons
  5. Meyer, B. (1988) Bidding farewell to globals. JOOP(Journal of Object-Oriented Programming) , pp.73-77.
  6. Hevery, M. (2008) Brittle Global State & Singletons [online]. Available from: http://misko.hevery.com/code-reviewers-guide/flaw-brittle-global-state-singletons/
  7. Yaiser, M. (2011) Object-oriented programming concepts: Objects and classes [online]. Available from: http://www.adobe.com/devnet/actionscript/learning/oop-concepts/objects-and-classes.html
  8. Caromel, D. (1993) Toward a method of object-oriented concurrent programming. Communications of the ACM , pp.90-102.
  9. Summit, S. (1997) Visibility and Lifetime (Global Variables, etc.) [online]. Available from: https://www.eskimo.com/~scs/cclass/notes/sx4b.html
'resource', 'type', 'prefix', 'path', 'host', 'schemes', 'methods', 'defaults', 'requirements', 'options', 'condition', 'controller', 'name_prefix', 'trailing_slash_on_root', 'locale', 'format', 'utf8', 'exclude', 'stateless', ]; private $yamlParser; /** * Loads a Yaml file. * * @param string $file A Yaml file path * @param string|null $type The resource type * * @return RouteCollection A RouteCollection instance * * @throws \InvalidArgumentException When a route can't be parsed because YAML is invalid */ public function load($file, string $type = null) { $path = $this->locator->locate($file); if (!stream_is_local($path)) { throw new \InvalidArgumentException(sprintf('This is not a local file "%s".', $path)); } if (!file_exists($path)) { throw new \InvalidArgumentException(sprintf('File "%s" not found.', $path)); } if (null === $this->yamlParser) { $this->yamlParser = new YamlParser(); } try { $parsedConfig = $this->yamlParser->parseFile($path, Yaml::PARSE_CONSTANT); } catch (ParseException $e) { throw new \InvalidArgumentException(sprintf('The file "%s" does not contain valid YAML.', $path), 0, $e); } $collection = new RouteCollection(); $collection->addResource(new FileResource($path)); // empty file if (null === $parsedConfig) { return $collection; } // not an array if (!\is_array($parsedConfig)) { throw new \InvalidArgumentException(sprintf('The file "%s" must contain a YAML array.', $path)); } foreach ($parsedConfig as $name => $config) { $this->validate($config, $name, $path); if (isset($config['resource'])) { $this->parseImport($collection, $config, $path, $file); } else { $this->parseRoute($collection, $name, $config, $path); } } return $collection; } /** * {@inheritdoc} */ public function supports($resource, string $type = null) { return \is_string($resource) && \in_array(pathinfo($resource, PATHINFO_EXTENSION), ['yml', 'yaml'], true) && (!$type || 'yaml' === $type); } /** * Parses a route and adds it to the RouteCollection. * * @param string $name Route name * @param array $config Route definition * @param string $path Full path of the YAML file being processed */ protected function parseRoute(RouteCollection $collection, string $name, array $config, string $path) { $defaults = isset($config['defaults']) ? $config['defaults'] : []; $requirements = isset($config['requirements']) ? $config['requirements'] : []; $options = isset($config['options']) ? $config['options'] : []; foreach ($requirements as $placeholder => $requirement) { if (\is_int($placeholder)) { throw new \InvalidArgumentException(sprintf('A placeholder name must be a string (%d given). Did you forget to specify the placeholder key for the requirement "%s" of route "%s" in "%s"?', $placeholder, $requirement, $name, $path)); } } if (isset($config['controller'])) { $defaults['_controller'] = $config['controller']; } if (isset($config['locale'])) { $defaults['_locale'] = $config['locale']; } if (isset($config['format'])) { $defaults['_format'] = $config['format']; } if (isset($config['utf8'])) { $options['utf8'] = $config['utf8']; } if (isset($config['stateless'])) { $defaults['_stateless'] = $config['stateless']; } $route = $this->createLocalizedRoute($collection, $name, $config['path']); $route->addDefaults($defaults); $route->addRequirements($requirements); $route->addOptions($options); $route->setHost($config['host'] ?? ''); $route->setSchemes($config['schemes'] ?? []); $route->setMethods($config['methods'] ?? []); $route->setCondition($config['condition'] ?? null); } /** * Parses an import and adds the routes in the resource to the RouteCollection. * * @param array $config Route definition * @param string $path Full path of the YAML file being processed * @param string $file Loaded file name */ protected function parseImport(RouteCollection $collection, array $config, string $path, string $file) { $type = isset($config['type']) ? $config['type'] : null; $prefix = isset($config['prefix']) ? $config['prefix'] : ''; $defaults = isset($config['defaults']) ? $config['defaults'] : []; $requirements = isset($config['requirements']) ? $config['requirements'] : []; $options = isset($config['options']) ? $config['options'] : []; $host = isset($config['host']) ? $config['host'] : null; $condition = isset($config['condition']) ? $config['condition'] : null; $schemes = isset($config['schemes']) ? $config['schemes'] : null; $methods = isset($config['methods']) ? $config['methods'] : null; $trailingSlashOnRoot = $config['trailing_slash_on_root'] ?? true; $namePrefix = $config['name_prefix'] ?? ''; $exclude = $config['exclude'] ?? null; if (isset($config['controller'])) { $defaults['_controller'] = $config['controller']; } if (isset($config['locale'])) { $defaults['_locale'] = $config['locale']; } if (isset($config['format'])) { $defaults['_format'] = $config['format']; } if (isset($config['utf8'])) { $options['utf8'] = $config['utf8']; } if (isset($config['stateless'])) { $defaults['_stateless'] = $config['stateless']; } $this->setCurrentDir(\dirname($path)); /** @var RouteCollection[] $imported */ $imported = $this->import($config['resource'], $type, false, $file, $exclude) ?: []; if (!\is_array($imported)) { $imported = [$imported]; } foreach ($imported as $subCollection) { $this->addPrefix($subCollection, $prefix, $trailingSlashOnRoot); if (null !== $host) { $subCollection->setHost($host); } if (null !== $condition) { $subCollection->setCondition($condition); } if (null !== $schemes) { $subCollection->setSchemes($schemes); } if (null !== $methods) { $subCollection->setMethods($methods); } if (null !== $namePrefix) { $subCollection->addNamePrefix($namePrefix); } $subCollection->addDefaults($defaults); $subCollection->addRequirements($requirements); $subCollection->addOptions($options); $collection->addCollection($subCollection); } } /** * Validates the route configuration. * * @param array $config A resource config * @param string $name The config key * @param string $path The loaded file path * * @throws \InvalidArgumentException If one of the provided config keys is not supported, * something is missing or the combination is nonsense */ protected function validate($config, string $name, string $path) { if (!\is_array($config)) { throw new \InvalidArgumentException(sprintf('The definition of "%s" in "%s" must be a YAML array.', $name, $path)); } if ($extraKeys = array_diff(array_keys($config), self::$availableKeys)) { throw new \InvalidArgumentException(sprintf('The routing file "%s" contains unsupported keys for "%s": "%s". Expected one of: "%s".', $path, $name, implode('", "', $extraKeys), implode('", "', self::$availableKeys))); } if (isset($config['resource']) && isset($config['path'])) { throw new \InvalidArgumentException(sprintf('The routing file "%s" must not specify both the "resource" key and the "path" key for "%s". Choose between an import and a route definition.', $path, $name)); } if (!isset($config['resource']) && isset($config['type'])) { throw new \InvalidArgumentException(sprintf('The "type" key for the route definition "%s" in "%s" is unsupported. It is only available for imports in combination with the "resource" key.', $name, $path)); } if (!isset($config['resource']) && !isset($config['path'])) { throw new \InvalidArgumentException(sprintf('You must define a "path" for the route "%s" in file "%s".', $name, $path)); } if (isset($config['controller']) && isset($config['defaults']['_controller'])) { throw new \InvalidArgumentException(sprintf('The routing file "%s" must not specify both the "controller" key and the defaults key "_controller" for "%s".', $path, $name)); } if (isset($config['stateless']) && isset($config['defaults']['_stateless'])) { throw new \InvalidArgumentException(sprintf('The routing file "%s" must not specify both the "stateless" key and the defaults key "_stateless" for "%s".', $path, $name)); } }}