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

Revert "optimize loading plugins - use only composer autoloader" #2

Closed
wants to merge 1 commit into from

Conversation

glensc
Copy link
Collaborator

@glensc glensc commented May 6, 2019

This reverts commit 01f21b2 it looks to me as a premature optimization what is not well tested.

This actually broke setup of the application here,
and keeping this in does not seem to break anything else (I've been using many years zf1 version in several applications without problems).


The helpers used to be loaded from application/views/helpers automatically on demand by Zend framework, removing this support breaks applications.

Message: Plugin by name 'ImageUrl' was not found in the registry; used paths: Zend_View_Helper_: Zend/View/Helper/:/app/application/views/helpers/

A workaround would be to add application/views/helpers to composer classpath autoloader exists, but that's incompatible change, requiring to make extra changes in projects.

Also, that approach creates another problem: class names may conflict, and composer will pick only first matching class.

Not particularly with view helpers, but controllers are loaded (to my knowledge) in the same manner:

Warning: Ambiguous class resolution, "ErrorController" was found in both "application/modules/ccc-grabber/default/controllers/ErrorController.php" and "application/modules/default/controllers/ErrorController.php", the first will be used.
Warning: Ambiguous class resolution, "IndexController" was found in both "application/modules/ccc-grabber/default/controllers/IndexController.php" and "application/modules/default/controllers/IndexController.php", the first will be used.

cc @falkenhawk

This reverts commit 01f21b2.

This actually broke setup of application here,
and keeping this does not seem to break anything else.
@falkenhawk
Copy link
Member

falkenhawk commented May 6, 2019

@glensc you are right, what worked for us, may not work for others depending on original zf architecture and its loader. I'll consider merging this in. btw we decided to completely switch to composer autoloader for performance reasons, while migrating projects to use composer packages. This required:

  1. namespacing all classes so that they won't get in conflict on the global namespace,
  2. putting additional composer.json files inside module folders, describing paths to load classes e.g.
{
    "autogenerated": "remove this property when you make changes in this file to avoid overwriting it",
    "autoload": {
        "psr-4": {
            "Model\\Core\\": "models/",
            "Front\\Core\\Component\\": "components/",
            "Front\\Core\\Event\\": "events/",
            "Front\\Core\\Helper\\": "helpers/",
            "Front\\Core\\Plugin\\": "plugins/",
            "Front\\Core\\Service\\": "services/",
            "Front\\Core\\Traits\\": "traits/",
            "Front\\Core\\View\\Helper\\": "views/helpers/",
            "Front\\Core\\View\\Filter\\": "views/filters/",
            "Front\\Core\\": "controllers/"
        }
    }
}
  1. telling composer to merge all the composer.json files scattered across modules with composer-merge-plugin: (in main composer.json file)
	"require": {
		"wikimedia/composer-merge-plugin": "^1.3",
	},
	"extra": {
		"merge-plugin": {
			"include": [
				"application/modules/*/composer.json",
				"admin/application/modules/*/composer.json"
			]
		},
	},

The modules' composer.json files were auto-generated with such ugly-written tool:
6-modules-composer.zip

As this technique was not written down and described anywhere in detail, I will probably revert the change, as suggested by you, letting the zend/loader do its fallback job. But this won't help with the case with conflicting classnames on the global space anyway... (i'd suggest namespacing them if possible)

@falkenhawk
Copy link
Member

falkenhawk commented May 6, 2019

note: we gained big performance improvements thanks to switching to composer autoloader completely, esp. when we could have dumped composer's autoloader with "classmap-authoritative" option afterwards on production. I can't cite detailed numbers anymore because those perf tests results got lost somewhere, but I remember that letting that zend-loader scan all those paths and checking if a file exists etc. summed up to pretty significant number of extensive filesystem operations.

@falkenhawk
Copy link
Member

@glensc Would you consider this as a compromise? #4

@glensc
Copy link
Collaborator Author

glensc commented May 7, 2019

@falkenhawk i'm ok with #4.

@falkenhawk
Copy link
Member

replaced with #4

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

Successfully merging this pull request may close these issues.

2 participants