First up, LOVE the module.
I'm trying to use the image functions from compass, like say: image-height, but they don't seem to work.
After a little bit of debugging and looking at the callstack I think I've tracked the possible bug down to sassy_compass__resolve_path which looks for the filepath in the context of the current SassScriptFunction, but it seems that the context never gets passed from the SassScriptParser.
Making this simple change gets it working, but I've no idea what ramifications that change might have.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | sassy-1509822-image-dimensions_23.patch | 11.77 KB | joseph.olstad |
| #10 | sassy-1509822-image-dimensions-just-fix.patch | 6.15 KB | steven jones |
| #8 | sassy-1509822-image-dimensions.patch | 14.67 KB | steven jones |
| #8 | sassy-1509822-image-dimensions-just-fix.patch | 5.14 KB | steven jones |
| #5 | sassy-1509822-image-functions-2.patch | 525 bytes | steven jones |
Comments
Comment #1
steven jones commentedPatch attached.
Comment #2
richthegeek commentedHi, that patch is great - we might be able to do it "better" by using the same style that "image-url" etc use for resolving a file, but either way this bug will be resolved in the next push.
The whole compass stuff needs to be tested more thoroughly really, so if you feel like helping out more then that would be a great place to start!
Comment #3
steven jones commentedOh I'd missed that way of doing it entirely.
Comment #4
richthegeek commentedPrimary advantage of being the guy who wrote 90% of the code = knowing 90% of the code!
Comment #5
steven jones commentedOkay, copying the usage of
sassy_compass__urlfromsassy_compass__image_url.Comment #6
richthegeek commentedLooks good to me! I'll test it out and roll it in when I get a chance.
Thanks!
Comment #7
steven jones commentedGoing to have a look at this one.
Comment #8
steven jones commentedHere's a patch that tests the URL functions a little, the first includes the test fixes from #1532042: Add a base test class and the second is a patch from that point.
Comment #10
steven jones commentedTry that again.
Comment #12
steven jones commentedNeeds to be a binary patch.
Comment #14
steven jones commentedLet's see what the testbot thinks of this.
Comment #16
steven jones commentedI think we need to use more of Drupal's functions to build the URLs here.
Comment #17
joseph.olstadSteven Jones, just want to say great work on this patch.
Unfortunately while it may have passed testing in 2012 I was unable to get it to pass testing today. I tried merging the code to the current build with similar test results as if reverting to a dev on the same date of the latest patch however there's exception messages on the test runs . Actually I got better results with applying the patch to todays build however it looks like we'd have to figure out what changed or not in phpsass (the library)
I believe that the exception messages are not necessarily an issue with the patch as it was but more about the changes upstream in the phpsass library and possibly in prepro as well (more likely in phpsass) , your patch is maybe referring to a function call that no longer behaves as it did back in august of 2012.
For this reason I am marking this as "postponed". If someone wants to pick up where you left off they may, seems like quite a bit of work to get the patch working again without reverting older problems that I just fixed today. Sad to see your hard work go to waste on this , maybe someone will pick this up later, it'll need a big refactor and a lot of testing.
Comment #18
joseph.olstadchange it to postponed. if someone is brave and has time to spare, maybe they can continue this issue.
Comment #19
joseph.olstad@Steven Jones did some great work here.
Rerolled his patch... my bad the previous time I wasn't using git to apply the binary part, this time it should work.
setting this to needs review
Comment #21
joseph.olstadComment #23
joseph.olstadComment #25
joseph.olstadComment #26
joseph.olstadThanks @Steven Jones ! great work, I just did a very small amount.
Comment #28
joseph.olstadWay to go @Steven Jones,
I amended the commit to give author credit to @Steven Jones
Thanks again!