Download & Extend
CSS » Issues

No support for versions

Posted by Crell on January 14, 2009 at 7:16am
Project:CSS
Version:6.x-1.x-dev
Component:Code
Category:task
Priority:normal
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

The current implementation does not support node versions at all. It needs to, as otherwise there is no way to roll back CSS changes along with a revision.

Login or register to post comments

Comments

#1

Posted by fax8 on January 19, 2009 at 10:02am
Assigned to:Anonymous» fax8

I agree. I'm working on this one.

#2

Posted by fax8 on January 19, 2009 at 11:13am

The attached patch implements support for node revisions in css module.

Please test this patch. These are the tests I'm interested:

  • DB schema update process (running update.php)
  • New DB creation (brand new install)
  • CSS module uninstall process
  • Node creation with CSS
  • Node creation without CSS
  • Node update without new CSS
  • Node update with new CSS
  • Node update with new CSS and new revision
  • Node update withot new CSS and new revision
  • Node delete
  • Node revision delete
  • Node revision revert
AttachmentSize
css.revisions.patch 6.66 KB

#3

Posted by fax8 on January 19, 2009 at 11:38am

The above patch change the behaviour of using @import "?q=css/get/x"; to reference the CSS rules of other nodes.
Before x was the nid (node id) while it is now the vid (version id).

I'm not sure if it could be useful also a way to reference the latest CSS version of a css attached to a node.

With the implementation in the patch if one reference a CSS attached to a specified version of a node, and then a new version of the referenced css is available, nodes referencing the old version will be not updated with the new versions.

Probably the best workaround would be provide another hook (eg. css/get_nid/x) where x is now the nid. This hook would always returns the latest revision of the CSS.

Any other idea?

#4

Posted by Crell on January 19, 2009 at 4:46pm
Status:active» needs review

I'd actually argue that we should move away from the dynamic call, as that results in two hits against Drupal. Instead, do what imagecache does a and feed in a path to drupal_add_css() (that may need to be uncacheable) that points to $files_dir/nodecss/$nid, which in turn is a menu callback that generates a file at that location. That would avoid the second hit against Drupal itself on subsequent requests. That's probably a separate issue, though.

It probably does make sense to go by vid here, though. I'll try to test this patch when sometime soon.

#5

Posted by whispero on March 13, 2009 at 9:49am

Hi,

Just chasing this up.. Has this patch been tested yet and if so what feedback if any is there?

- Whispero

#6

Posted by Crell on March 13, 2009 at 2:52pm

Unfortunately I had a number of things explode on me in the past few weeks so I've not been able to do so. I'll let you know when I do. :-/

#7

Posted by whispero on March 23, 2009 at 3:07pm

Ok Crell,

No probs

- Whispero

#8

Posted by fax8 on June 29, 2009 at 7:55am

Still looking for feedback for the patch above. Anyone helps?

#9

Posted by Crell on March 19, 2010 at 4:14am
Status:needs review» needs work

Right, well, the patch in #2 no longer applies anyway, so I can't test it.

#10

Posted by Crell on July 2, 2010 at 3:33am
Version:6.x-1.0-rc1» 6.x-1.x-dev
Status:needs work» needs review

Right, so it looks like the patch in #2 was written for Drupal 5 to begin with. Here's a new version for Drupal 6. I also did some documentation cleanup while I was in there to better match Drupal's documentation standards because my brain was having a parse error otherwise. :-)

AttachmentSize
revisions.patch 5.53 KB

#11

Posted by fax8 on July 2, 2010 at 7:00am
Category:bug report» task
Priority:critical» normal
Assigned to:fax8» Anonymous
Status:needs review» needs work

Thanks Crell. Unfortunately there is a problem in the update routine. Same problem also on my D5 patch.

We both did a simple

UPDATE {css} SET vid = nid

While this works for nodes where revisions have never been used it fails if there is a revisions: it happens that nid != vid and thus data css is lost (it actually remains attached to the first revision, I think).

#12

Posted by fax8 on July 5, 2010 at 9:46am
Status:needs work» needs review

Ok Larry, I think I fixed the problems in your patch. Here are the changes I did on your code:

  • the .install UPDATE query you used was UPDATE {css} SET vid = nid.
    I modified it to be UPDATE css c SET vid = (SELECT vid FROM node n WHERE n.nid = c.nid) which should fix the problem I explained above
  • You didn't changed anything in the load op of hook_nodeapi so that in your patch you used nid to load the css. This is incorrect as it would always load the first css revision available. Changed to use vid instead of nid
  • In css_get() we both queried with a join on node table. This will permits to get results only if the version is the current active. If it isn't (eg while browsing an old revision in the revisions tab) it won't work. Changed the query to join on node_revisions rather than node.

Attached patch against D6-dev with the above fixes. Users interested in getting this features into css module please carefully test this.

Thanks,

Fabio Varesano

AttachmentSize
adding_revisions_support-358605-1-fax8.patch 5.9 KB

#13

Posted by jdipper on November 27, 2010 at 10:01pm

The only 'bug' (if we want to call it that) that I could find is if the following has happened:

1. A revision for a node (with css) is created (i.e. rev 1 -> rev 2).
2. The patch is applied
3. The node is then reverted back to rev 1.

Rather than the CSS block not being reverted (i.e. the content remains the same as rev 2), it 'reverts' to an empty text box.

Effectively, you lose the CSS info if you revert to a version that was made prior to the patch was created.

Honestly, I am not sure if this is a bug that can be fixed. Perhaps a check could be added to see if a valid css historical revision exists - but that might get complicated.

Other than that, the patch seems to work.

Jack

#14

Posted by fax8 on November 28, 2010 at 8:57am

Of course it won't work doing that.. anyway, I don't think this is something we should take care of.

Thanks for testing the patch, I'm going to include it in the next revision.

#15

Posted by jdipper on November 28, 2010 at 2:51pm

Agreed, it would be fairly hard to work out the logic. Although it's probably best we warn folks that reverting back to a node revision prior to installing the update will delete the css.

Jack

#16

Posted by jdipper on February 11, 2011 at 12:45pm

Quick question... when can we expect a new revision?

Jack

#17

Posted by Crell on June 7, 2011 at 10:28pm
Status:needs review» fixed

I have committed #12 along with an update to the install.txt file regarding the edge case noted above. It should be in the 6.x-1.x branch as soon as it regenerates.

fax8, when do you want to roll a new release?

#18

Posted by fax8 on June 8, 2011 at 9:02am

I think we can roll out a release for 6.x right now.. the code has been under testing for quite some time now and I think it's ok for a stable 6.x version. You should have the permissions to roll out a release, please do that if you have time. Otherwise, I'll do that, just let me know.

Regarding 7.x, I think I do agree with the people asking for merging with Code Per Node.

#19

Posted by System Message on June 22, 2011 at 9:11am
Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.