Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA: Replacement test case for 3rd party class containing ZendForm in its full path #94

Closed
wants to merge 1 commit into from

Conversation

patrick-braun
Copy link

Signed-off-by: Patrick Braun patrick.braun@krumedia.com

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

Corresponding test to issue #93
Adds unit test that is currently failing because the replacement rules replace the word ZendForm in 3rd party paths, which it should not do.

How to reproduce

Try to use a custom class that contains "ZendForm" somewhere in its class path, it'll get renamed to LaminasForm.

Expected:

CustomZendFormBinder => CustomZendFormBinder

Actual:

CustomZendFormBinder => CustomLaminasFormBinder

Signed-off-by: Patrick Braun <patrick.braun@krumedia.com>
@Ocramius Ocramius added the Bug Something isn't working label Apr 19, 2022
@Ocramius
Copy link
Member

Does it just get a new class alias though? Shouldn't break anything, unless you run into weird collisions 🤔

@freshmeat13
Copy link

The problem is that the 3rd Party Library, which is contained in the vendor folder, is not renamed through the laminas migrate command. The Laminas migration thinks, that the class name was modified and changes it in the configuration.
Because this is happing at runtime through the \Laminas\ZendFrameworkBridge\ConfigPostProcessor you cannot repair it after the migration.

return array(
	'controller_plugins' => array(
		'factories' => array(
			'customZendFormBinder' => \CustomZendFormBinder\Controller\Plugin\Factory\BinderPluginFactory::class,
		),
	),
);

After post processor:

return array(
	'controller_plugins' => array(
		'aliases' => array(
			'customZendFormBinder' => 'customLaminasFormBinder' ,
		)
		'factories' => array(
			'customLaminasFormBinder' => \CustomLaminasFormBinder\Controller\Plugin\Factory\BinderPluginFactory::class,
		),
	),
);

At runtime the autoloader tries to load CustomLaminasFormBinder\Controller\Plugin\Factory\BinderPluginFactory.
This would be ok, if the class was part of the project. It's namespace would have been changed through the laminas migrate call. But because it's a 3rd-party-library from the vendor folder, it's not altered and the factory cannot be found.

Possible solutions:

  1. Be very specific with renaming classes to reduce the risk of renaming a class accidentally. This should have been the solution in the initial state of the bridge tool, but I see the problem of changing this in the late state the module is in. The risk of damaging existing installations is higher than the benefit of fixing this bug.
  2. Rename the class in the 3rd party library. Obviously not a good solution, because you cannot be sure, that the library code is under control of the project maintainer. Also the 3rd party library loses the possibility to be used by zend and laminas code on the same version.
  3. Make it possible, that the project maintainer can add custom replacements to the Replacements class through global configuration. This is probably the most simple fix and it's very safe, because it does not effect existing installations. The change could look like this:
class ConfigPostProcessor
{
    public function __construct(array $additionalReplacements = [])
    {
        $this->replacements = new Replacements($additionalReplacements);
public function onMergeConfig($event)
    {
		/** @var ConfigMergerInterface */
		$configMerger = $event->getConfigListener();
		$mergedConfig = $configMerger->getMergedConfig($returnAsObject = false);

		$additionalReplacements = $mergedConfig['laminas-zendframework-bridge']['additionalReplacements'] ?? [];
		$processor = new ConfigPostProcessor($additionalReplacements);
		$configMerger->setMergedConfig(
			$processor(
				$mergedConfig
			)
		);
	}

The project maintainer is responsible for adding exceptions to the renaming process. It looks like this was planned, because Replacements already has a parameter $additionalReplacements, but it's not possible set it at the moment.

What do you think?
Is solution 3 acceptable to you? Then we would prepare a PR for it.
Or do you think a real fix (solution 1) would be preferable, even at the risk of affecting existing installations?
Solution 2 is out of the question for us. In this case, we will probably have to work with our own fork of the Laminas Zend Bridge.

@weierophinney weierophinney added this to the 1.6.0 milestone Jul 14, 2022
@weierophinney weierophinney self-assigned this Jul 14, 2022
weierophinney added a commit to weierophinney/laminas-zendframework-bridge that referenced this pull request Jul 14, 2022
Per a suggestion in laminas#94, this patch modifies the `ConfigPostProcessor` such that it now examines the merged configuration passed to it for its own configuration, specifically for replacement rules.
These may be provided via the following configuration:

```php
return [
    'laminas-zendframework-bridge' => [
        'replacements' => [
            'to-replace' => 'replacement',
            // ...
        ],
    ],
];
```

This configuration itself will NEVER be rewritten.
However, the rules will now be used as additional replacements when running the processor.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants