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

No way for a theme or extension to import LESS files from vendor directory #4218

Closed
leoquijano opened this issue Apr 18, 2016 · 15 comments
Closed
Labels
feature request Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@leoquijano
Copy link

Hi,

As described in #2264, there's currently no way to import LESS files from a theme or extension directory to the vendor/ folder. This is particularly relevant for theme developers since most custom themes could include content from different LESS libraries, and those libraries are better maintained using Composer or Bower (Bower can be configured to also put packages inside vendor/).

So suppose I have this in my theme's main.less file:

@import '../../../../../../../vendor/twbs/bootstrap/less/bootstrap.less';

Now, I could use Bootstrap compiled CSS, but that wouldn't allow me to use variables and mixins from that deployment. If I try to copy Bootstrap files directly in the theme web directory, then the LESS preprocessor will pick them up and fail, since it tries to compile every file in the Bootstrap library instead of just the main one (which imports the other ones).

(And also, it's better to keep external CSS libraries in the vendor directory anyway)
The error shown is this:

File path '../../../../../../vendor/twbs/bootstrap/less/bootstrap.less' is forbidden for security reasons.>

I tried to symlink the file (which is not always the best solution since some deployments may fail to reproduce symlinks adequately, especially on Windows), but it's not working either. I'll probably have to copy the full Bootstrap code in a file as a workaround, but it's more maintainable to enable some way of importing from vendor/ directory.

@leoquijano leoquijano changed the title Now way for a theme or extension to import LESS files from vendor directory No way for a theme or extension to import LESS files from vendor directory Apr 18, 2016
@vrann vrann added the PS label Apr 18, 2016
@mazhalai
Copy link
Contributor

Thank you for reporting @leoquijano, we have created ticket MAGETWO-52044 to investigate and fix.

@mazhalai mazhalai added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Apr 19, 2016
@BenSpace48
Copy link
Contributor

I had a similar problem, my workaround was to include Bootstrap in my theme directory, and rename all the bootstrap files to include an underscore at the start. Then import _bootstrap.less in my theme's _extend.less.

It's far from ideal and I am getting some errors when running a static content deploy which could be related to my workaround.

@mazhalai
Copy link
Contributor

mazhalai commented Apr 29, 2016

Closing this since it was fixed in the develop branch, in the scope another internal ticket MAGETWO-48840 See commit 98eb8f8

@leoquijano
Copy link
Author

leoquijano commented Apr 29, 2016

Hi @mazhalai , I think commit #98eb8f8 actually fixes #2264?

My report is different, as it is directed to /vendor directory and not the Magento UI library. I was requested to open a new issue, and here it is.

@leoquijano
Copy link
Author

I double checked, and yes, that commit only fixes #2264 .

$directoryWeb = $this->readFactory->create(
  $this->getDirectoryList()->getPath(DirectoryList::LIB_WEB)
);
$fileRead = $this->readFactory->create($realPath);
// Check if file path starts with web lib directory path
if (strpos($fileRead->getAbsolutePath(), $directoryWeb->getAbsolutePath()) === 0) {
  return

There's no provision for importing things from the vendor directory.

@MomotenkoNatalia
Copy link
Contributor

Yes this issue exists and now it's not possible to use 3rd party composer packages web assets. Thank you for pointing out to this problem, architects will work on finding solution for this use case.

@leoquijano
Copy link
Author

Can I suggest enabling a configurable set of directory prefixes? It would default to "Lib::" and "Vendor::". I'm thinking about people who might want to use Bower or other provisioning tools.

Also, I see that the issue is closed. Is there a way to track progress on it?

@giacmir
Copy link
Member

giacmir commented Sep 12, 2016

Why is this issue closed? I don't see any evidence that it has been solved or rejected.

@pboisvert
Copy link

@tkacheva can ou review this one and reopen if original concern is not addressed?

@tkacheva tkacheva reopened this Oct 19, 2016
@tkacheva
Copy link

@leoquijano Currently there is no such feature in Magento to reference web assets of 3rd party library installed via Composer. We have issue opened in the backlog MAGETWO-52044 to add this option.

@giacmir
Copy link
Member

giacmir commented Oct 20, 2016

Actually this is not a "new Magento feature", but a standard less behaviour that Magento has broken by customizing the less pipeline. I tend to consider this a bug, not a feature request.

@piotrekkaminski
Copy link
Contributor

Thank you for your submission.

We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues.

Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

We are closing this GitHub ticket and have moved your request to the new forum.

@alankent
Copy link

Hi @giacmir - just to clarify on "its a bug", it is defined (and important) Magento behavior. There is a directory structure to CSS (and Less) files that a web browser sees. One of the currently supported modes is "client side Less compilation". It can only see the web server directory structure. So the structure of files as visible via a web browser is the "true" directory structure as seen by Less.

Magento has the concept of "file fall backs". The file in the highest priority module or theme wins. So a new theme or module can customize existing behavior by replacing that file. This works by having a "search path" - all paths are tried relative to each module and theme until the file is found. (Gulp and I think Grunt has this search path concept as well. It can be used for theme inheritance in non-Magento projects as well.)

For example, each module has a 'web' directory (e.g. App/code/Magento/Customer/view/frontend/web). These are all merged into one directory tree (I am not repeating the full rules here - I can dig up the dev docs link if needed.)

There is a command, useful with Grunt and Gulp, which creates pub/static by creating symlinks back to the original files. It resolves all file fall back processing. This ends up with the pub/static directory tree of Less files in exactly the right directory structure for client side Less compilation to do all the path resolution.

So the requested change is to break existing functionality of file overriding in Magento. Relative paths are not file system paths - they are web server paths after fall back processing. (Or "search path" processing if you prefer - same concept.)

However, we agree that it would be good to pull in 3rd party CSS, JS etc libraries via Composer more easily. The problem is how to get them into the Magento path namespace, put into the web directory, etc. We cannot change relative paths to have a different meaning - that would break current sites. So its more like we need some way to register 3rd party composer packages with JS/CSS/whatever files into the Magento file system space. That would be the feature request aspect of this request.

@giacmir
Copy link
Member

giacmir commented Oct 25, 2016

Thanks @alankent for your clarification.

I don't want to be polemic, but I still think that this issue represent a broken functionality.
And the point is exactly where you say "we need some way to register 3rd party composer packages with JS/CSS/whatever". This "some way" is exactly what the @import directive in less is for.

For server side compilation the required less files are collected by a Resolver class.

In #2264 this Class has been modified so that it recognizes "././" as a relative pointer to the Magento Ui library. (and this seems like a horrible hack). It is important that that class is able to recognize other references as well so that we don't have to use a menomated LESS syntax.

Obviously this behaviour has to be replicated in gulp/grunt tooling.

Said that, I just want to mention that most of the complains about frontend would be solved by not having fallback for less. This may seem an anti-pattern in the Magento perspective but the problem is that CSS already has its own fallback mechanisms (c is for "cascading"). A more rational approach instead of this hell would be to have a nice set of libraries that a frontend dev can import and override locally using language native features.

You say "that would break current sites", but I recently upgraded from 2.0 to 2.1 and my theme was badly broken by the upgrade... so... yes... it will.. but in this stage it already breaks everything among major releases. I sincerely ask you to consider to remove fallback from less for standardization, simplicity, ease of use and peace of mind.

@leoquijano
Copy link
Author

leoquijano commented Oct 25, 2016

I agree with @giacmir and I should mention that given the current state of upgrading, we're expecting Magento to break functionality between minor versions anyway, so this is a good update to include in 2.2.0

magento-team pushed a commit that referenced this issue Jun 25, 2019
Fixed Issues:
- MAGETWO-99567: Update Url ActionList
- MC-16365: [2.1.x] Fix CompanyUserManagerInterfaceTest failing on mainline
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests

10 participants