[dev] CSS Parsing class

[Michael M Slusarz]

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Michael M Slusarz <slusarz at horde.org>:
>
>> Quoting Jan Schneider <jan at horde.org>:
>>
>>> Can we instead keep a conditional in the client code until the 5.1  
>>> release which checks whether Horde_Css_Parser exists and falls  
>>> back to Csstidy if not?
>>
>> Csstidy is not used in client code other than IMP.  And as  
>> mentioned above, using the new code in IMP fixes at least one  
>> security-related issue (better described as a privacy issue, but  
>> still).
>>
>> And in Horde_Core... we most certainly don't want to be doing some  
>> sort of horde application version check >= 5.1.  That sort of  
>> defeats the whole purpose of separating applications and PEAR  
>> packages.
>
> Not a version check, but a simple "if  
> (class_exists('Horde_Css_Parser'))" switch.

But how would this work?  IMP 6.0.5 is going to require  
Horde_Css_Parser. There can be no argument about that, since it fixes  
a security/privacy issue (along with another minor formatting issue).   
If the fix requires 1 line or 1000, it shouldn't matter.  The question  
is: does the new code fix the problem without breaking anything else?   
The answer is yes.

So obviously once IMP requires it, Horde_Core is going to use it using  
this approach.  And it doesn't make sense to say everybody not running  
IMP should use different code in Horde_Core than people running IMP.

>> It also demonstrates a limitation with our current git layout.  For  
>> example... I really want to update jquerymobile to 1.3.0 to allow  
>> for implementation of the IMP smartmobile tablet view.  But there's  
>> not really a coherent way to accomplish this.
>
> Create a topic branch.

This won't work for a variety of reasons.

Unfortunately, the only way this can work going forward is if either  
1) IMP has it's own version of jquerymobile or 2) we add, not replace,  
jquerymobile 1.3 to Horde_Core and then provide a way for an  
application to decide which version it wants to use.  But that's a  
different topic.

>>> We can make Css_Parser an optional dependency then and release  
>>> beta versions first for people to test this more thoroughly. With  
>>> 5.1 we completely switch to the new parser.
>>
>> I've already tested and verified via diff tool comparison that the  
>> new code is already outputting the same output for translating URLs  
>> and is correctly recognizing/importing external stylesheets.  So  
>> its already doing everything the current code does.  Anything else  
>> will just be a bonus.
>
> It's still a complete replacement of a whole library in a minor  
> bugfix version. This is unacceptable. 5.1 is not far away, there is  
> absolutely no need for any pressure to get this out now.

So what?  A bug fix is a bug fix.  Just because one bug fix changes  
more code than another bug fix doesn't mean it should wait.

As mentioned previously: I have gone through EVERY released  
application to compare the output using a graphical diff tool.   
**EVERYTHING** csstidy does the new code does.  Again: this has been  
verified but actually looking at the output.  Not to mention the  
upstream code has extensive unit tests to ensure the code works  
properly.  Whether the new code breaks someone's personal CSS  
additions is irrelevant. Because there are other people out there - as  
seen on the list - that have valid CSS code that doesn't work  
currently.  In this scenario, either way is broken.  The new way is  
just less broken.

(For the record: all the changes made since the original import of the  
upstream code are either optimizations or CSS3 fixes, neither of which  
affects the behavior of the current released code).

If you have noticed, I have not yet removed the 'nocache' parameter  
from the smartmobile view.  The final upstream changes I sent  
yesterday now make it appear that we can remove this.  I do want to  
test this a bit more because, due to the nature of jquerymobile, CSS  
issues can actually break the application.  again - this isn't an  
issue with the other views because I have verified that the output  
code is at least as clean as what the old csstidy code produced.

And what happens when a major distribution (say Debian) decides to  
package Horde_Core 2.4.4?  We have two years of people complaining  
about a bug that was fixed well before that version was released.

Lastly, csstidy is a mess.  We shouldn't even pretend it is stable  
code.  I've already duct taped several fixes since 5.0, and these  
fixes turned out to not be fully correct.  So the idea that we are  
somehow replacing "stable" code is simply incorrect.

michael

___________________________________
Michael Slusarz [slusarz at horde.org]