Closed boesing closed 2 years ago
This package is considered feature-complete, and is now in security-only maintenance mode, following a decision by the Technical Steering Committee. If you have a security issue, please follow our security reporting guidelines. If you wish to take on the role of maintainer, please nominate yourself
This one is a bit more tricky than some of the others, in that it provides implementations (both of ContainerInterface
and the "not found" exception type). If somebody were type-hinting against container-interop and expected classes in this component to then be matched, we could run into issues.
The classes of concern:
Laminas\Crypt\Exception\NotFoundException
(implements container-interop NotFoundException
)Laminas\Crypt\Symmetric\PaddingPluginManager
(implements container-interop ContainerInterface
)Laminas\Crypt\Symmetric\SymmetricPluginManager
(implements container-interop ContainerInterface
)I'm not terribly worried; these are all really internal classes, not intended for extension. The NotFoundException
is thrown by the internal implementations, and the two plugin managers are for use by the classes in this component only, and should never have additional plugins added to them. The classes that consume the plugin managers do checks for ContainerInterface
internally, so converting to PSR-11 is a type-widening scenario for them.
BC breakage from this change is slim, but it's still a possibility, and I wonder if it would warrant a 4.0 of this component, particularly as this is a security-focused component. @Ocramius , what are your thoughts?
@Ocramius
IMO two approaches possible:
container-interop/container-interop
in 3.x
, bumping its minimum required version thoughcontainer-interop
in laminas/laminas-servicemanager:4.0.0
laminas/laminas-servicemanager:4.0.0
is neededYes, it's annoying/painful, but the PaddingPluginManager
and SymmetricPluginManager
implementing ContainerInterface
are problematic.
psr/container
in a minor release (still BC compliant)We could remove the container-interop/container-interop
dependency:
And replace it with a new version of laminas/laminas-servicemanager
, which effectively unifies ContainerInterface
into a single interface.
This way, the package:
container-interop/container-interop
container-interop
with psr/container
in a MINOR releaseThis should be BC compliant, but I haven't mentally explored all ramifications.
In fact, I think that after some review of my thought process by @weierophinney we can:
laminas/laminas-servicemanager:^3.11.2
as dependencycontainer-interop/container-interop
Interop\Container
references with Psr\Container
referencesThis SHOULD be safe, given laminas/laminas-servicemanager
provides and replaces container-interop
, but somebody needs to try it :D
Added 3.8.0
milestone to reflect that this can be done without breaking BC, thanks to @boesing's work.
Hm, feels a bit tough to add the servicemanager dependency just for the sake of replacing the interfaces here. FYI: servicemanager dep was removed in 2015 with 2.6.0: https://github.com/zendframework/zend-crypt/pull/4
But since we would only require the servicemanager here, which' only dependency is laminas-stdlib
(which is also a dependency from this component), I think that might be fine as a temporary solution.
I've created a branch locally that does as @Ocramius suggests. I then created a small project with the following composer.json
:
{
"repositories": [
{
"type": "path",
"url": "/home/matthew/git/laminas/laminas-crypt"
}
],
"require": {
"container-interop/container-interop": "^1.2",
"laminas/laminas-crypt": "dev-feature/container-interop-removal"
}
}
and the following test script:
<?php
use Interop\Container\ContainerInterface;
use Interop\Container\Exception\NotFoundException;
use Laminas\Crypt\BlockCipher;
use Laminas\Crypt\Symmetric\Mcrypt;
use Laminas\Crypt\Symmetric\Openssl;
require './vendor/autoload.php';
$pluginManager = new class() implements ContainerInterface {
private const PLUGINS = [
'mcrypt' => Mcrypt::class,
'openssl' => Openssl::class,
];
/**
* @param string $name
* @return bool
*/
public function has($name)
{
return array_key_exists($name, self::PLUGINS);
}
/**
* @param string $name
* @return object
*/
public function get($name)
{
if (! $this->has($name)) {
$message = sprintf('The symmetric adapter %s does not exist', $name);
throw new class($message) extends RuntimeException implements NotFoundException {};
}
$class = self::PLUGINS[$name];
return new $class();
}
};
BlockCipher::setSymmetricPluginManager($pluginManager);
$cipher = BlockCipher::factory('openssl');
printf("Created cipher of type %s\n", $cipher::class);
$r = new ReflectionObject($pluginManager);
printf("Plugin manager implements: %s\n", implode(', ', $r->getInterfaceNames()));
printf(
"Plugin manager %s implement %s\n",
$pluginManager instanceof ContainerInterface ? 'DOES' : 'DOES NOT',
ContainerInterface::class
);
This produces the following output:
Created cipher of type Laminas\Crypt\BlockCipher
Plugin manager implements: Psr\Container\ContainerInterface
Plugin manager DOES implement Interop\Container\ContainerInterface
Knowing this, I think we're good to go; creating the PR shortly.
Feature Request
Summary
This issue is created to keep track of the status of getting rid
container-interop
in thelaminas
components.