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

do we need the composer-installer package as part of the project? #554

Closed
Flyingmana opened this issue Oct 16, 2018 · 18 comments
Closed

do we need the composer-installer package as part of the project? #554

Flyingmana opened this issue Oct 16, 2018 · 18 comments

Comments

@Flyingmana
Copy link
Contributor

Someone was complaining (a few years ago already, but just saw it) that we require a specific composer installer while there are multiple different ones people use.

So I wonder, do we actually need it as part of the project, or can we remove this requirement?

@tmotyl
Copy link
Contributor

tmotyl commented Oct 17, 2018

I prefer to have it in. It lowers the barrier. If somebody knows a better alternative than what we use, I would like to hear the arguments.

@Flyingmana
Copy link
Contributor Author

for which user-flows is it reducing the barrier?
we should at least document this to have a place to link if someone asks.

@tmotyl
Copy link
Contributor

tmotyl commented Oct 17, 2018

It is reducing the barier for people migrating from non-composer to composer way of installing magento.

@Flyingmana
Copy link
Contributor Author

if this is the exact usecase: the installer is not needed/usefull for installing magento. This feature was split up into a different project which is dedicated to just installing the core.

@LeeSaferite
Copy link
Contributor

The installer is useful for installing the project. If someone has an issue with the installer used, they can solve that by a replace entry in their project or by having their other installer indicate that it replaces the one used by default

@Flyingmana
Copy link
Contributor Author

Thats the point which was made, and which is true. The installer has absolutely no effect on installing the project, its only usefull for additional magento modules.

@colinmollenhour
Copy link
Member

colinmollenhour commented Oct 24, 2018

What is the suggested way of managing an installation based on composer? It seems that it would require an app/Mage.php (or similar) hack to require 'vendor/autoload.php' and that if we're going to have a composer.json file at all it should support composer-installed packages without requiring hacks to any core files.

Edit: It would be great to see an example project that requires this project as well as other third-party extensions, and perhaps add such an example to the README.

@colinmollenhour
Copy link
Member

colinmollenhour commented Oct 24, 2018

What about adding a block of code like the following to app/Mage.php right after Varien_Autoload::register(); so that standard packages can have their autoloaders loaded properly and automatically?

/* Support additional includes, such as composer autoloaders */
foreach (glob(BP . DS . 'app' . DS . 'etc' . DS . 'includes' . DS . '*.php') as $path) {
    include_once $path;
}

For example I want to include "aws/aws-sdk-php" package and this package requires a specific file to be loaded in additional to the psr-4 class autoloader being registered:

{
...
    "autoload": {
        "psr-4": {
            "Aws\\": "src/"
        },
        "files": ["src/functions.php"]
    },
...
}

So without something like the suggestion above how do people handle loading this file properly in the Magento bootstrap process?

@Flyingmana
Copy link
Contributor Author

from our own install page: http://openmage.github.io/magento-lts/install.html
we seem to expect people to not checkout the repository directly, but to install it as a dependency.

So managing the core installation is possible (and documented) completely without using the installer we have. Additionally the AOE installer is I think including the module installation functionality(at least someone mentioned to prefer the aoe installer above the hackathon one)

questions which are currently open:

  • what is the suggested way to install Magento Modules
  • do we want to support non magento modules (aka general composer packages )

We then should split this up into dedicated Issues to focus on a single topic.
Especially Issues for writing documentation for certain parts of this, so we later can point people to it.

Currently the hackathon composer installer is hacking/patching the core to load the composer autoloader.
Easier would it be, if we add a check, if OpenMage is installed in context of composer (which is not so easy, as we dont know where the vendor is located in relation)
I actually like the glob approach, but maybe is one file per Project enough, as we (I guess) dont want to add another internal API for modules to bootstrap things.

@colinmollenhour
Copy link
Member

colinmollenhour commented Oct 24, 2018

I tried and really like the aoepeople/composer-installers installer since it allows you to use symlinks just as if you were using only modman which for development is extremely helpful. However, I couldn't see that it was including the vendor/autoload.php file anywhere in Magento core code so I'm guessing this is left up to the user.

It seems like there are many ways to do this and forcing one single way is perhaps not necessary. However, there is also an opportunity to settle on a good solution which works great out of the box and is easy for everyone to adopt.

Using the glob include approach it would make it easy to integrate any number of methods of integrating composer and most importantly, allow it to be done without modifying core files. E.g. the aoepeople installer could be forked/updated to symlink the vendor/autoload.php into app/etc/includes/aoepeople-composer-installer.php so that it works with no file modifications with my proposed app/Mage.php update.

In case anyone wants to try this here is what I did:

$ mkdir magento-composer-test
$ cd magento-composer-test
$ cat <<JSON > composer.json
{
    "require": {
        "aoepeople/composer-installers": "*",
        "openmage/magento-lts": "1.9.3.x-dev",
        "aoepeople/aoe_scheduler": "dev-master"
    }
}
JSON
$ composer install
$ echo "htdocs" > .modman/.basedir
$ modman deploy-all

You end up with the Magento root in htdocs and the Aoe_Scheduler module files symlinked into the Magento directories correctly. The only thing lacking is a require somewhere for the vendor/autoload.php file which with the proposed patch would be one more step for the user or for a custom installer:

$ ln -s ../../../../vendor/autoload.php htdocs/app/etc/includes/composer.php

@colinmollenhour
Copy link
Member

Regarding the original question, (sorry to sidetrack) since it seems other installers routinely use the "replace" keyword with this installer this shouldn't be a big deal to have it as a requirement. I could see changing it in the future to one that completes the autoloader integration automatically, though.

edannenberg pushed a commit to edannenberg/magento-lts that referenced this issue Dec 6, 2018
…ntegration

Example use:

$ ln -s ../../../../vendor/autoload.php htdocs/app/etc/includes/composer.php

refs: OpenMage#554
edannenberg pushed a commit to edannenberg/magento-lts that referenced this issue Feb 14, 2019
…ntegration

Example use:

$ ln -s ../../../../vendor/autoload.php htdocs/app/etc/includes/composer.php

refs: OpenMage#554
fplantinet pushed a commit to fplantinet/magento-lts that referenced this issue Mar 27, 2019
edannenberg pushed a commit to edannenberg/magento-lts that referenced this issue Apr 1, 2019
…ntegration

Example use:

$ ln -s ../../../../vendor/autoload.php htdocs/app/etc/includes/composer.php

refs: OpenMage#554
rafaelpatro pushed a commit to rafaelpatro/magento-lts that referenced this issue May 14, 2019
@IbrahimS2
Copy link

@colinmollenhour @Flyingmana How would this solution work with the compiler? would not be better to incorporate this with what exists in such a way we do not conflict with what exists? I was thinking of Magento 2 approach in this regard being honest!

@colinmollenhour
Copy link
Member

the compiler?

You mean Mage Compiler? I thought we removed this?

@IbrahimS2
Copy link

@colinmollenhour Oh, I did not know that I joined this repo only recently. Why is that though? it used to boost up the platform performance by having all the necessary files in one place.

Could you put some light on this matter?

@colinmollenhour
Copy link
Member

See #534

edannenberg pushed a commit to edannenberg/magento-lts that referenced this issue Aug 22, 2019
…ntegration

Example use:

$ ln -s ../../../../vendor/autoload.php htdocs/app/etc/includes/composer.php

refs: OpenMage#554
edannenberg pushed a commit to edannenberg/magento-lts that referenced this issue Oct 25, 2019
…ntegration

Example use:

$ ln -s ../../../../vendor/autoload.php htdocs/app/etc/includes/composer.php

refs: OpenMage#554
edannenberg pushed a commit to edannenberg/magento-lts that referenced this issue Aug 20, 2020
…ntegration

Example use:

$ ln -s ../../../../vendor/autoload.php htdocs/app/etc/includes/composer.php

refs: OpenMage#554
@func0der
Copy link

func0der commented Mar 7, 2021

I like to bring this back up as I got this comment from @AydenHassan today: AydinHassan/magento-core-composer-installer#57 (comment)

There should be a fork of his project or the AOE one, if OpenMage wants to continue using those, of course with proper attribution.
This would not only look more "professional" and make it easier to start with OpenMage, but also would enable the community around OpenMage to contribute to the project no matter what.

@AydinHassan
Copy link

AydinHassan commented Mar 7, 2021

Btw it's not that the project is dead or abandoned - I'm just not actively working on it. Like I said I am happy to merge PR's but don't really want to spend time debugging it (I don't think that warrants a fork, someone will have to do the work either way, if it is truly a bug). Further to that the project has been stable for years and as far as I can remember the only real recent change was for composer 2 support.

@Flyingmana
Copy link
Contributor Author

I think the initial topic is not relevant anymore. Most of the existing composer installers are not made compatible for Composer2 and the interest in them seems not be so big.
And to require the one installer by default seems to be the currently preferred solution. So would see the initial topic as finished so I will close the Ticket.

regarding the Installer from AydinHassan, there would be no advantage in forking it, rather the opposite as long as Aydin is still taking care of PRs. And we dont really have spare resources for this currently.
Anyway, this would belong into a different discussion.

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

7 participants