richthegeek / phpsass

A compiler for SASS/SCSS written in PHP, brought up to date (approx 3.2) from a fork of PHamlP: http://code.google.com/p/phamlp/
http://phpsass.com/
382 stars 83 forks source link

Variables cannot be changed from within a for loop #62

Open chrisinajar opened 11 years ago

chrisinajar commented 11 years ago

If you define a variable in a function, you cannot change it from within a for loop. Example code is below.

@mixin test-mixin(val) {
  left: test-function(val);
}

@function test-function($var) {
  $inner-var: "Wrong!";

  @for $i from 1 through 1 {
    $inner-var: "It worked!";
  }

  @return $inner-var;
}

body {
  @include test-mixin('woot');
}

If i replace the line inside the for loop with a return statement then the correct value appears, so I know the loop is running.

chrisinajar commented 11 years ago

If I comment out line 92 of SassForNode.php it appears to work

$context = new SassContext($context);

I'm not sure if there are adverse affects of simply passing the same context in, but it works...

richthegeek commented 11 years ago

I was just thinking it's probably a context issue! I think it's actually an issue with how variables are handled generally.

The way variables work now (similar to how PHP handles it):

  1. a variable assign occurs ($x: 42)
  2. the variable x gets set to 42 in the current SassContext

The way it should actually work (similar to how JS handles it):

  1. a variable assign occurs ($x: 42)
  2. the parents of the current SassContext are searched for existance of $x
  3. if it exists, change the value of the variable within that context
  4. if it does not exist, set it within the local context.

Of course, it could simply be a case of loops not requiring their own contexts! The only issue I can think of (which is not really an issue) would be the lop variables potentially "bleeding" out of their intended context.

chrisinajar commented 11 years ago

That makes sense. I looked at the context parenting code a little bit, I think your description of how it should work is exactly on.

If I find the time I'll take a stab at fixing this issue, otherwise I'll be watching closely for a fix from you whenever you similarly find the time (I know how that can be)

I think that for loops should have their own contexts, however removing that line is a quick fix for now to keep things going. I'm trying to get Bourbon to work but it's proving much more difficult that I had expected. Hopefully I can make a few more pull requests / bug reports as I continue to work through the Bourbon issues.

Thanks!

drsii commented 9 years ago

@chrisinajar did you ever get bourbon to work?