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

[WIP][RFC] improve plugin loading with symfony1-plugin typed composer packages #342

Open
connorhu opened this issue Mar 20, 2024 · 2 comments

Comments

@connorhu
Copy link
Collaborator

connorhu commented Mar 20, 2024

For Issue #341, I looked into what is needed to put sfDoctrinePlugin in a separate package and not have it shipped with the base framework (e.g. propel orm doesn't need this plugin). I don't really like the solution of the other fos1 plugin packages. What happens there is that composer-installer 1.0 knows about symfony1-plugin and copies the installed package into the appropriate plugins directory. This is not very practical to do in the composer era. Let's take a look at what to do:

  • the sfProjectConfiguration::getAllPluginPaths method should return all packages that are of type symfony1-plugin:
          if (class_exists(Composer\InstalledVersions::class)) {
              $sf1PluginPackageNames = Composer\InstalledVersions::getInstalledPackagesByType('symfony1-plugin');
    
              foreach ($sf1PluginPackageNames as $sf1PluginPackageName) {
                  $path = Composer\InstalledVersions::getInstallPath($sf1PluginPackageName);
    
                  if ($path !== null) {
                      $pluginPaths[basename($path)] = $path;
                  }
              }
          }
  • we should not allow composer/installers 1.0 to be in the dependencies, because this will duplicate the plugins

This change would get rid of package codes (which don't work anyway: https://github.com/orgs/FriendsOfSymfony1/discussions/333#discussion-6326793 ), pear codes, and should be default the composer installation way of the framework and the plugins.

What other improvements could be considered?

  • support psr-4 plugins?
  • composer plugin to enable plugins (same as bundle.php).

Files to remove

  • ./package.xml.tmpl
  • ./lib/plugin/*
  • ./test/unit/plugin/*

Deprecated commands/tasks

  • plugin:add-channel
  • plugin:install
  • plugin:uninstall
  • plugin:upgrade

Deprecated methods

  • sfBaseTask::getPluginManager
@alquerci
Copy link
Contributor

alquerci commented Mar 31, 2024

Good idea.

Another idea without composer dependency.
But using ReflectionClass.

It avoids plugin duplication

we should not allow composer/installers 1.0 to be in the dependencies, because this will duplicate the plugins

If a plugin is installed with composer, it is also autoloaded.
Then

class ProjectConfiguration extends sfProjectConfiguration
{
    public function setup()
    {
        $this->enablePlugins([
            \Some\Vendor\SomePluginConfiguration::class,
        ]);
    }
}

where

    public function getPluginPaths()
    {
        if (!isset($this->pluginPaths[''])) {
            $pluginPaths = $this->getAllPluginPaths();

            $this->pluginPaths[''] = [];
            foreach ($this->getPlugins() as $plugin) {
                if (isset($pluginPaths[$plugin])) {
                    $this->pluginPaths[''][] = $pluginPaths[$plugin];
                } else {
                    // START new code
                    try {
                        $pluginConfigurationClass = $plugin;
                        $pluginReflection = new ReflectionClass($pluginConfigurationClass);

                        $classFile = $pluginReflection->getFileName();

                        // to be compatible with loadPlugins()
                        // require_once '${pluginPath}/config/${pluginName}Configuration.class.php'; be noop
                        $pluginName = $this->extractBaseClassWithoutConfigurationSuffix($pluginConfigurationClass);
                        $pluginPath = $this->extractPluginDirectory($classFile);

                        $this->pluginPaths[''][] = $pluginPath;
                        $this->setPluginPath($pluginName, $pluginPath);
                    } catch (ReflectionException) {
                         throw new InvalidArgumentException(sprintf('The plugin "%s" does not exist.', $plugin));
                    }
                    // END new code
                }
            }
        }

        return $this->pluginPaths[''];
    }

@connorhu
Copy link
Collaborator Author

connorhu commented Apr 4, 2024

I have nothing against relying heavily on the composer. It has become a quasi-industry standard. Currently, the code relies heavily on an outdated package management system (pear). This code won't live to see composer become obsolete, so integrating it won't be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants