aerialship / SamlSPBundle

src/AerialShip/SamlSPBundle/Config/EntityDescriptorFileProvider.php

<?php

namespace AerialShip\SamlSPBundle\Config;

use AerialShip\LightSaml\Model\Metadata\EntitiesDescriptor;
use AerialShip\LightSaml\Model\Metadata\EntityDescriptor;
use Symfony\Component\HttpKernel\KernelInterface;

class EntityDescriptorFileProvider implements EntityDescriptorProviderInterface
{
    /** @var  KernelInterface */
    protected $kernel;

    /** @var  string */
    protected $filename;

    /** @var  string|null */
    protected $entityId;

    /** @var  EntityDescriptor|null */
    private $entityDescriptor;



    function __construct(KernelInterface $kernel)

It is generally recommended to explicitly declare the visibility for methods.

    {
        $this->kernel = $kernel;
    }


    /**
     * @param string $filename
     * @throws \InvalidArgumentException
     */
    public function setFilename($filename)
    {
        if ($filename && $filename[0] == '@') {
            $filename = $this->kernel->locateResource($filename);
        }
        if (!is_file($filename)) {
            throw new \InvalidArgumentException('Specified file does not exist: '.$filename);
        }
        $this->filename = $filename;

It seems like $filename can also be of type array. However, the property $filename is declared as type string. Maybe add an additional type check?

    }

    /**
     * @return string
     */
    public function getFilename()
    {
        return $this->filename;
    }

    /**
     * @param null|string $entityId
     */
    public function setEntityId($entityId)
    {
        $this->entityId = $entityId;
    }

    /**
     * @return null|string
     */
    public function getEntityId()
    {
        return $this->entityId;
    }




    /**
     * @return EntityDescriptor
     */
    public function getEntityDescriptor()
    {
        if ($this->entityDescriptor === null) {
            $this->load();
        }
        return $this->entityDescriptor;
    }


    protected function load()
    {
        $doc = new \DOMDocument();
        $doc->load($this->filename);
        if ($this->entityId) {

The expression $this->entityId of type string|null is loosely compared to true; this is ambiguous if the string can be empty. You might want to explicitly use !== null instead.

            $entitiesDescriptor = new EntitiesDescriptor();
            $entitiesDescriptor->loadFromXml($doc->firstChild);

$doc->firstChild of type object<DOMNode> is not a sub-type of object<DOMElement>. It seems like you assume a child class of the class DOMNode to be always present.

            $this->entityDescriptor = $entitiesDescriptor->getByEntityId($this->entityId);
        } else {
            $this->entityDescriptor = new EntityDescriptor();
            $this->entityDescriptor->loadFromXml($doc->firstChild);

$doc->firstChild of type object<DOMNode> is not a sub-type of object<DOMElement>. It seems like you assume a child class of the class DOMNode to be always present.

        }
    }
}