Fix compilation issues with Bootstrap 3

[jimmyphong]

thanks ! :+1:

[damienalexandre]

Here is some informations about this issue, tested with Bootstrap 3.0.2 (latest stable).

lessc->zipSetArgs() receive this:

// args
array(2) {
  [0]=>
  array(2) {
    [0]=>
    string(3) "arg"
    [1]=>
    string(6) "@index"
  }
  [1]=>
  array(2) {
    [0]=>
    string(3) "arg"
    [1]=>
    string(5) "@list"
  }
}
// orderedValues
array(1) {
  [0]=>
  array(3) {
    [0]=>
    string(6) "number"
    [1]=>
    string(1) "1"
    [2]=>
    string(0) ""
  }
}
// keywordValues
array(0) {
}

And throw this error: Failed to assign arg @list: line: 57.

This is a case where:

• there is no kerword arg
• no ordered tag
• no default value

Here is the faulty LESS code, from mixins.less:

// Framework grid generation
//
// Used only by Bootstrap to generate the correct number of grid classes given
// any value of `@grid-columns`.

.make-grid-columns() {
  // Common styles for all sizes of grid columns, widths 1-12
  .col(@index) when (@index = 1) { // initial
    @item: ~".col-xs-@{index}, .col-sm-@{index}, .col-md-@{index}, .col-lg-@{index}";
    .col(@index + 1, @item);
  }
  .col(@index, @list) when (@index =< @grid-columns) { // general; "=<" isn't a typo
    @item: ~".col-xs-@{index}, .col-sm-@{index}, .col-md-@{index}, .col-lg-@{index}";
    .col(@index + 1, ~"@{list}, @{item}");
  }
  .col(@index, @list) when (@index > @grid-columns) { // terminal
    @{list} {
      position: relative;
      // Prevent columns from collapsing when empty
      min-height: 1px;
      // Inner gutter via padding
      padding-left:  (@grid-gutter-width / 2);
      padding-right: (@grid-gutter-width / 2);
    }
  }
  .col(1); // kickstart it
}

.make-grid-columns-float(@class) {
  .col(@index) when (@index = 1) { // initial
    @item: ~".col-@{class}-@{index}";
    .col(@index + 1, @item);
  }
  .col(@index, @list) when (@index < @grid-columns) { // general
    @item: ~".col-@{class}-@{index}";
    .col(@index + 1, ~"@{list}, @{item}");
  }
  .col(@index, @list) when (@index = @grid-columns) { // terminal
    @{list} {
      float: left;
    }
  }
  .col(1); // kickstart it
}

Hope it helps; from my side, if I comment the $this->throwError("Failed to assign arg " . $a[1]); line from zipSetArgs, it works as expected.

I suspect this error is thown to prevent calling a mixing without a mandatory param, but here, there is a "when", if we call .col with 1 as first param, the @list can be unset, it's not needed.

[dalabarge]

You just need to add .col to the failing mixins:

- .col(1); // kickstart it
+ .col(1, '.col'); // kickstart it

That said, LessPHP isn't compatible with the "Mixings With Multiple Parameters" syntax. Specifically:

It is legal to define multiple mixins with the same name and number of parameters. Less will use properties of all that can apply. For example, if you used the mixin with one parameter e.g. .mixin(green);, then properties of all mixins with exactly one mandatory parameter will be used

Bootstrap is calling the .col mixin which has been defined three times, twice of which include a mandatory parameter. It appears that LessPHP is always passing the @list parameter but without a value as there is none defined in fact using the last definition of the .col mixin (which includes the @list parameter. It appears the bug lies in the guard handling. If the block's $args were filtered by the mixin's required args and the key unset if the @list argument was undefined and also not required, then everything would be just fine. Lack of code commentary means I couldn't figure it out in a reasonable amount of time.

Adding .col selector as the @list value tricks the compiler by providing a default value while also introducing the helpful .col selector that matches the first .col-xs-1, ..., .col-lg-1 selectors.

I doubt Bootstrap will accept this as a patch since it clearly works with Less.js without it; but for those who want to run a patch until LessPHP can catch up then here you go. It's unfortunate that LessPHP is so poorly commented as otherwise many here would have been able to help keep it compatible. It's also very unfortunate that Bootstrap community refuses to support the PHP community by providing assistance and work arounds: Less.js isn't the only compiler out there.

[jonvan]

This compiler appears to work with bootstrap 3: https://github.com/oyejorge/less.php

[maubut]

+1, BS3.0.2

[dalabarge]

@mweimerskirch this a well commented "solution", however, it's not a good practice to remove something from the code unless it's not needed and in this case it is an exception that is there for good reason: when you forget to pass a required parameter to a mixin this throws an exception. The consequences of just removing that line could appear elsewhere so I would not recommend this solution to anyone else finding this same issue. It also has the invalid semantic HTML element selector null at the same CSS rule you have for .col-xs-1 which isn't particularly good either. By removing the exception you break LessPHP and also invalidate your CSS.

[dmathisen]

@bexarcreative-daniel I tried your fix and it worked, but now I get a new error .calc-grid is undefined. Same thing mentioned here: https://github.com/leafo/lessphp/issues/491#issuecomment-28252687 Bootstrap 3.0.2; LessPHP 0.4.0

[dalabarge]

I'm not sure what your configuration is but I too am using 0.4.0 and Bootstrap 3.0.2 (even dev-master works). The only changes to the mixin I made are where it says "kickstart it".

// Framework grid generation
//
// Used only by Bootstrap to generate the correct number of grid classes given
// any value of `@grid-columns`.

.make-grid-columns() {
  // Common styles for all sizes of grid columns, widths 1-12
  .col(@index) when (@index = 1) { // initial
    @item: ~".col-xs-@{index}, .col-sm-@{index}, .col-md-@{index}, .col-lg-@{index}";
    .col(@index + 1, @item);
  }
  .col(@index, @list) when (@index =< @grid-columns) { // general; "=<" isn't a typo
    @item: ~".col-xs-@{index}, .col-sm-@{index}, .col-md-@{index}, .col-lg-@{index}";
    .col(@index + 1, ~"@{list}, @{item}");
  }
  .col(@index, @list) when (@index > @grid-columns) { // terminal
    @{list} {
      position: relative;
      // Prevent columns from collapsing when empty
      min-height: 1px;
      // Inner gutter via padding
      padding-left:  (@grid-gutter-width / 2);
      padding-right: (@grid-gutter-width / 2);
    }
  }
  .col(1,'.col'); // kickstart it
}

.make-grid-columns-float(@class) {
  .col(@index) when (@index = 1) { // initial
    @item: ~".col-@{class}-@{index}";
    .col(@index + 1, @item);
  }
  .col(@index, @list) when (@index < @grid-columns) { // general
    @item: ~".col-@{class}-@{index}";
    .col(@index + 1, ~"@{list}, @{item}");
  }
  .col(@index, @list) when (@index = @grid-columns) { // terminal
    @{list} {
      float: left;
    }
  }
  .col(1,'.col'); // kickstart it
}

The .calc-grid may not be being used by my particular LESS compilation which is why I don't experience the error.

The truth of it all is that in the effort to save the Bootstrap framework developer a bit of time (let's face it programmers are lazy) they've used code that looks nothing like CSS by using the most complex syntax of LESS. If you look at the compiled output, it could just have easily been built using a bit simpler howbeit more verbose LESS syntax and then more compilers wouldn't choke on it.

[yansern]

See my comment here for the temporary .calc-gridfix. Use it along with the .col(1, true) fix and "bootstrap/grid" should compile just fine. https://github.com/leafo/lessphp/issues/491#issuecomment-31226429

[seven-phases-max]

Would it be easy for someone to create a new fork of bootsrap with the recommended fixes?

Isn't that easier to switch to less.php?

[elboletaire]

Isn't that easier to switch to less.php?

I did it when @jonvan suggested it a few comments above :wink:

[jnilla]

I was not aware of this project, that is the official php port, what is the difference compared with leafo's work

[Roope]

There is no official. These are just two different php ports.

[yansern]

I work on a large-scale less project that has lots of less files in multiple-level of folders and does quite a bit of variable overriding.

We switched to oyejorge's less.php for a few days. While we've got all the obvious migration-related issues fixed, we just could not get it to compile properly. We could not get past the issue of "Recursive variable definition" when a variable that references other variable gets feeded through a mixin in separate files.

Does this code below look legit to you? Well, it doesn't compile with oyejorge's less.php.

// file 1
@backgroundColor: #999;
@panelBackgroundColor: @backgroundColor;

// file 2
.someMixin(@color) { background: darken(@color, 5%); }

// file 3
.someClass { .someMixin(@panelBackgroundColor); }

Also, we have a .transform() mixin that throws an error, we had to rename to .transform_().Sometimes we get errors like Warning: First parameter must either be an object or the name of an existing class. and we could not figure out why.

Oyejorge strives to follow less.js' standards and that is admirable but I guess we're too comfortable with leafo's lessphp so we switched back. At least we were able to make Bootstrap 3.0.3 compile with a few hacks.

However, I fear if leafo doesn't come up with &:extend() support soon it definitely won't compile with Bootstrap later than > 3.0.3. And judging from the specs of &:extend(), it doesn't look easy to implement to me.

[seven-phases-max]

Well, I'd say that reporting an issue to the less.php (which is already in its LESS 1.5.1 betas) could look a bit more rational than pinging the lessphp (which is not even close to LESS 1.4.x) with "when?"s. But I'm not using either of two personally, so it's just an ignorant observation from an external universe :)

[jnilla]

@seven-phases-max @elboletaire i don't meant to be rude guys but if that project is not official either what is the point of telling people to move to that project, why not trying to makes this project better and fix its problems. Telling other people to move to another project of the same nature in a issue thread seem to me very rude and also a bit spammy

[seven-phases-max]

@enav I'm not quite sure what this all is having to do with being "official" or not. But either way, well, I'd say it's quite subjective on your side to call someone a spammer (do you realize that neither me nor @elboletaire are directly involved with either of two projects?) just because she make a purely neutral suggestion trying to save some people from wasting their time?

[leafo]

I know I'm a little late to this party but as you've probably noticed I don't have much time for this project anymore. I'm actually more of a SCSS guy instead of a LESS guy anway. :smile:

Does anyone want commit access to this repo? The stuff that is missing for BS3 is actually fairly minimal. Biggest show stopper for latest less version is extends, if it's the same as SCSS then I've already implemented it for https://github.com/leafo/scssphp

[dmathisen]

Re: https://github.com/leafo/lessphp/issues/503#issuecomment-31332372

Tagging people who have helped on this issue... maybe one of them is willing to get commit access and solve this. @damienalexandre @bexarcreative-daniel @jstonne @enav

[dalabarge]

If this is no longer going to be maintained by the original creator then I would say that trying to come up with a more official version perhaps by combining leafo/lessphp with oyejorge/less.php as a community effort would be in store.

@leafo I'd be happy to have commit access though my time would prohibit me from being able to address the compatibility issues. If some of the other guys would want to begin making leafo/lessphp less.js v1.5 compatible and do it in the PSR-0/1 standard then I'll collaborate.

In the mean time I've switched to using node.js's less compiler and incorporating it into my pre-deploy build process. Not as purist but the node.js community is maintaining this sort of stuff much better... At the end of the day I just needed less compiling and it didn't matter which language so I jumped from lessphp to leafo/lessphp and now onto node's lessc.

[yansern]

@bexarcreative-daniel I'm in for a collaborative effort. Pinning this responsibility to a single person would be heavy.

[jnilla]

@leafo those are sad news but i respect your reasons, thanks for the awesome compiler anyways

[saas786]

@leafo sad news, you should at least manage the repo and let the people send commits etc. As there are lot of guys like me who love LESSCSS more then SCSS, I like how simple it is to use etc.

+1

[dalabarge]

@leafo: @jstonne and I have decided to try and implement the :extend syntax and also see about resolving this Bootstrap mixin incompatibility. Could we get a branch to work in or you want us working on our own fork? Also any idea where the best place is to tap into the compiler to provide the extend method? The code is a bit devoid of commentary...

[barryvdh]

@leafo Thanks for all the hard work you have done so far!

I only did see https://github.com/oyejorge/less.php/ untill someone mentioned it here. That versions compiles the latest bootstrap 3 version (looks fine on first sight) and seems more active.

Not to be rude or anything, but perhaps it would be better to focus everyone's energy on that library, if this isn't maintained anymore? Since a recent commit it is even compatible with the current interface (thus also the Assetic lessphp filter)

[mishal]

Hi all,

I've created a less parser using the oyejorge's code which supports the :extend syntax, compiles bootstrap3, generates sourcemaps and other stuff. Anyone is invited to cooperate!

The repo: https://github.com/mishal/iless

Cheers!

[barryvdh]

@mishal Is it a fork or a new project? Why not contribute it back to oyejorge's project?

[Roope]

I was thinking the same. Oyejorge's less.php seems to be in active development.

[mishal]

I thought that I will submit the changes I made back to @oyejorg 's repository, but had to change internals of the parser so it became incompatible with the original code and would cause BC breaks. Because of the implemented importers and other stuff I decided to roll-out a new project. I had to switch the name to prevent conflicts.

I've created unit tests for the code and the progress which was made on @oyejorge code was a bit faster than I could follow. I tried to solve the known problems in a non hackish way (file imports, usage of static properties, source maps,...)

[danyj]

OK I will be blunt for a sec , If anyone who is familiar with leafos code , knows how to fix this I will pay for it . The oyejorg version is not something I would go for.

[barryvdh]

Why not? Op 16 mrt. 2014 17:09 schreef "Dan" notifications@github.com:

OK I will be blunt for a sec , If anyone who is familiar with leafos code , knows how to fix this I will pay for it . The oyejorg version is not something I would go for.

Reply to this email directly or view it on GitHubhttps://github.com/leafo/lessphp/issues/503#issuecomment-37761130 .

[danyj]

@barryvdh I just tested it , server performance goes down the drain. Cache folder created is over 6.5MB and the logic on checking files or cache is just not well thought off.

Leafo's version gives you a simple array of files used in compile process with timestamps that you can check against real file times instead of making cache files and check them directly. Plus if I delete the cached files you got fatal error which should actually recompile again like leafo version does. And than the library size , compare to simple 86KB of leafos version. I dont see this being usefully for my or my customers servers.

Simply saying "it works" let me use it is not enough. Sorry.

[barryvdh]

Yeah but you dont run compilation on production websites I hope? I would prefer a closer match to the less.js version above speed any time.. But that is your choice. It does kind of work, when you remove the strict checking, or use a slightly older version. But it will get more and more behind the less.js version if this isn't maintained anymore.. Op 16 mrt. 2014 17:19 schreef "Dan" notifications@github.com:

@barryvdh https://github.com/barryvdh I just tested it , server performance goes down the drain. Cache folder created is over 6.5MB and the logic on checking files or cache is just not well thought off.

Leafo's version gives you a simple array of files used in compile process with timestamps that you can check against real file times instead of making cache files and check them directly. Plus if I delete the cached files you got fatal error which should actually recompile again like leafo version does. And than the library size , compare to simple 86KB of leafos version. I dont see this being usefully for my or my customers servers.

Reply to this email directly or view it on GitHubhttps://github.com/leafo/lessphp/issues/503#issuecomment-37761421 .

[danyj]

Sure I do , compilation runs on user level. Not auto. User clicks on compile , until bootstrap is compiled site is using default css files , so no harm done . If it is not able to compile for whatever reason site still works. Note that this is custom environment that does not depend on LESS , or bootstrap but if compiled bootstrap file is there than it will be used.

[barryvdh]

Yeah okay, that is what I meant, it isn't run on every request so speed isn't everything. And you can easily compare the filetime of the source with the compile time yourself. I believe there is even a class in oyegorge that mimics the cache behaviour from leafos version (and interface to make it easy to switch)

[danyj]

And I am not saying that the guys are doing something wrong. Comparison to less.js and code that works is always what I will go for. But if leafo has shown us anything than the simplicity of using files would be just that. Why does php compiler has to create dozens of cache files? Wouldn't simple array do the job?

[exside]

I see the problem in the much more complex implementation of &:extend/:extend from LESS (compared to SCSS) http://css-tricks.com/the-extend-concept/ anybody made some attempts to implement this?