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

Refactor code so modules/themes/language packs can exist under different directories (such as 'vendor') #1013

Closed
alankent opened this Issue Jan 30, 2015 · 5 comments

Comments

Projects
3 participants
@alankent
Copy link

alankent commented Jan 30, 2015

Currently we cannot leave modules (and themes and language packs) downloaded via Magento in the 'vendor' directory. We tried to convert the code, but hit a problem. TexanHogman (Rick) described it well here: #357 (comment).

The goal is to allow modules to reside under 'vendor' or 'app/modules' (renamed from 'app/code'). Currently there are file path patterns coded into the system (such as app/code///etc/module.xml) that make putting modules in different locations harder. (I personally would prefer to get the list of directories from the config.php file, worked out by the setup script, allowing modules to sit anywhere in the directory tree giving the developer greater freedom of how to lay out their code during development.)

The real problem is other areas of the code, like functional and integration tests, have too many dependencies on the current locations of files. One approach is to say "tests are only usable if you git clone the master repository". This is not ideal however. Production sites should never use git clone. They should use Composer to download official release versions of modules to ensure they are using a stable release version of the code. This is important for receiving Magento or extension developer support. So it is desirable to rework all the test cases to not depend on the current location of modules and other assets.

This work was started internally, but lowered in priority when the effort required to clean up the tests was fully understood. It is valuable, but there is other higher value / lower cost work that is being planned first. (Also note the internal project had a wider scope of changes than described in this request. This request focuses purely on allowing modules/themes/language packs to reside under 'vendor', with the goal of using Composer without modification or extension.) Offers of community contribution are welcome.

The following is a list of areas that may be affected as an illustration of types of changes likely to be needed:

  • dev/tests/static/framework/Magento/TestFramework/Utility/Files.php - quite a few paths
  • dev/tests/static/testsuite/Magento/Test/Integrity/App/Language/Package.php - glob calls
  • dev/tests/static/testsuite/Magento/Test/Integrity/App/Language/TranslationFiles.php - glob calls
  • The BP constant in the original source code has two meanings. One represents the root of the magento install, the other meaning is the directory from which source code can be found. This means all instances of BP need to be examined and either kept as is or replaced with code that uses the module directory. There were initially 140 usages of this constant.
  • setup/config/autoload/global.php - there is a 'module' directory, but the goal of this work is to allow modules to reside in multiple places so this concept probably needs to be replaced.
  • Most of the Magento product code uses autoloaders to access classes, but code in the the tests/, build/, tools/, and shell/ directories frequently access files by relative paths. There are 1780 uses of relative paths in project php files. A number of these will remain valid after file restructuring (because a whole subdirectory structure would be moved, so relative paths internal to that structure would remain valid), but our initial analysis underestimated the number of these that would need to be manually updated. These relative paths are commonly combined with variables meaning any attempt at scripting logical changes would be difficult.
  • Symbolic links not desirable because of usage of DIR in the code. DIR will be the physical location of a folder or a file. There are 1541 use cases of DIR in Magento code base.

The following is out of scope for this request:

  • XSD relative paths will break under 'vendor' directory, but that is a separate task to resolve. See #1012.

See also #357 for related discussion.

Given the potential complexity of this task, it is recommended to come up with a proposal to allow the root directory of each module be determined and get that reviewed before commencing with implementation work.

[Warning: This request is an experiment to see how well this approach works to outsourcing work and is subject to change or being withdrawn.]

@aadmathijssen

This comment has been minimized.

Copy link

aadmathijssen commented Apr 22, 2015

If I understand correctly, setting up a new Magento 2 application for an actual client site should be done as follows:

composer create-project magento/community-edition client_project_name -s dev

Ideally, this should put most code in the vendor folder, and as little files as possible in the root folder (similar to the way Symfony applications can be created with Composer).

Currently this is not the case, which poses a challenge for setting up version control: if you don't want to put the Magento 2 source code into version control, you have to create a pretty elaborate .gitignore (instead of just putting /vendor into your .gitignore).

Also, the current situation does not seem to allow performing upgrades to new versions of Magento 2 using composer update.

So I expect that these problems are fixed when this issue (#1013) is resolved, though I'm not sure.

@joanhe

This comment has been minimized.

Copy link
Contributor

joanhe commented Apr 22, 2015

@aadmathijssen I see there are couple of questions in your comments:

  1. About support to keep Magento 2 code under vendor. This problem should be resolved when this issue is resolved.
  2. Question about composer update. It doesn't work because the version in composer.json is fixed version, Magento 2 doesn't support a range of version yet. You can update the version you want in the composer.json, then run composer update.
    "name": "magento/product-community-edition",
    "description": "eCommerce Platform for Growth (Community Edition)",
    "type": "project",
    "version": "0.74.0-beta5",   <-----
@aadmathijssen

This comment has been minimized.

Copy link

aadmathijssen commented Apr 22, 2015

@joanhe Thanks for the reply!

Regarding the use of composer update, I think you need to do more than just change the version: it is possible that Magento makes other changes to the composer.json file, which should also be taken into account. Next to that, I probably also need to make changes to composer.json myself needed by the project specific parts (most likely extra requires). So the local composer.json is managed by two parties, which I think is not ideal.

@joanhe

This comment has been minimized.

Copy link
Contributor

joanhe commented Apr 22, 2015

@aadmathijssen That is correct. If Magento adds or removes some modules, it won't reflect in the composer.json. There is some change in next version. It should improve on this issue. You need to use package magento/project-community-edition to create project. The command in your case would be:
composer create-project magento/project-community-edition client_project_name -s dev

@aadmathijssen

This comment has been minimized.

Copy link

aadmathijssen commented Apr 22, 2015

@joanhe I hope the next version will improve on this issue, so people creating actual projects have a good starting point. I see that I have misspelled the package name in my first post; you're right, this should be magento/project-community-edition.

@magento-engcom-team magento-engcom-team moved this from TODO to Done in branch [2.3-develop] Sep 12, 2017

VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this issue Jun 22, 2018

Merge pull request magento#1013 from magento-engcom/1007
MSI-1007: Creating an order with a product that has a number in the product name causes fatal error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment