-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Composer plugin v3 #63
Draft
justinbeaty
wants to merge
19
commits into
MahoCommerce:main
Choose a base branch
from
justinbeaty:topic-composer-plugin-v3
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Composer plugin v3 #63
justinbeaty
wants to merge
19
commits into
MahoCommerce:main
from
justinbeaty:topic-composer-plugin-v3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fballiano
reviewed
Dec 6, 2024
…o into topic-composer-plugin-v3
justinbeaty
force-pushed
the
topic-composer-plugin-v3
branch
from
December 6, 2024 21:33
ea560a2
to
9339f89
Compare
justinbeaty
force-pushed
the
topic-composer-plugin-v3
branch
from
December 6, 2024 21:42
19f86b8
to
6f603e9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I made a couple more changes to improve the autoloading in Maho, feedback is welcome!
Cleanup app/Mage.php
First, I migrated the "startup" code over to
app/bootstrap.php
, so thatapp/Mage.php
is a pure class file.I also removed the
libxml_disable_entity_loader
stuff as it's been fixed since 2014. To quote a comment from 2020 in the linked PHP bug report: "If you're still using libxml < 2.9.0, better change that."Finally, I created a
lib/Maho.php
class instead of using functions defined infunctions.php
. So for example:Maho::findFile($file)
instead ofmahoFindFileInIncludePath($path)
.Pure autoloading
Everything can now be autoloaded by Composer like any other modern codebase, and you no longer need to require
app/bootstrap.php
andapp/Mage.php
.This makes
public/index.php
(and any other custom scripts) much simpler. Just require the autoloader, then you can use any class or function. For example:For legacy scripts you can still do it the old way, but if you require the autoloader AND require
app/Mage.php
you'll get an error: "Cannot declare class Mage, because the name is already in use". Simply changing therequire 'app/Mage.php'
to arequire_once
will fix that, but this scenario should be pretty rare.New utility functions
There's some new helper functions for working with installed modules:
Performance improvements
We gain a modest 1-2 ms as we don't need to build paths or scan
app/etc/includes/*.php
at runtime. These are pre-scanned during thecomposer dump
command.I believe there's also a possibly significant performance gain to be had with loading
phtml
files, but that's for another PR...Modman support
I added modman support to the composer plugin. For example assume you install Cm_Diehard via composer.
By default, you'll end up with files such as
vendor/colinmollenhour/cm_diehard/code/etc/config.xml
, but this isn't going to work with Maho.With modman support, the composer plugin will create symlinks in a special folder
vendor/mahocommerce/maho-modman-symlinks/$packageName
, for example:Then, if the composer plugin detects this folder, we'll use it as the module's install path instead. The feature will avoid creating symlinks if there's no actual difference in the source vs target files. For example modman files created with
generate-modman
don't need symlinks.It even supports modman's
@import
feature which was never supported in the OM plugin. That means you can define multiple modules in a single repo.I created some more test modules. Add the composer repo, then run:
composer require modman/testpage modman/simple modman/complex modman/nolink modman/composermap
Here's how the symlinks look -- Click to expand
In addition to modman support, it also supports composer.json
extra.map
.I also had written a package.xml parser, but I believe it's useless since I don't think you can define any custom mappings there. In other words,
app/code/local/My/Module
always links to the same location, which already works.Bug fixes
Before if you had simply created a folder like
app/code/core/Mage/Core/sql/core_setup
in a module or starter pack, it would have prevented the files inmahocommerce/maho
from running. This is fixed now. Technically you could override a single setup script, but of course that's not recommended.I'm pretty sure there was a discrepancy in the order that module's
etc/config.xml
files were loaded vs models, blocks, phtml files, etc. I didn't confirm this but it should be fixed now anyway. This would only cause problems if two composer packages define the same module like some 3rd party modules do (i.eAW_Core
) but one version was outdated and they weren't compatible.Other comments
The new composer plugin would need to be released as v3, as Maho 24.11.0 won't work with it.
Besides the possibility of the "Cannot declare class Mage" error, and the removal of the
mahoFindFileInIncludePath()
, etc functions, I don't believe there are any other breaking changes.I think
app/code/core/Mage/Core/functions.php
really should be atlib/Mage/functions.php
, but it's not really a big deal where it is now. Maybe eventually we can remove a lot of those functions anyway...Instead of
lib/Maho.php
we could put it somewhere likelib/Maho/Core.php
and have it loaded via PSR-4. So instead ofMaho::findFile()
it would be\Maho\Core::findFile()
. Might be useful in the long term if we want to split the file up into\Maho\Core
,\Maho\ErrorHandler
,\Maho\Whatever
, etc.