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.

CommentFileSizeAuthor
#12 adding_revisions_support-358605-1-fax8.patch5.9 KBfax8
#10 revisions.patch5.53 KBCrell
#2 css.revisions.patch6.66 KBfax8

Comments

fax8’s picture

fax8 commented
Assigned: Unassigned » fax8

I agree. I'm working on this one.

fax8’s picture

fax8 commented
StatusFileSize
new css.revisions.patch6.66 KB

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
fax8’s picture

fax8 commented

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?

Crell’s picture

Crell commented
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.

whispero’s picture

whispero commented

Hi,

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

- Whispero

Crell’s picture

Crell commented

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. :-/

whispero’s picture

whispero commented

Ok Crell,

No probs

- Whispero

fax8’s picture

fax8 commented

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

Crell’s picture

Crell commented
Status: Needs review » Needs work

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

Crell’s picture

Crell commented
Version: 6.x-1.0-rc1 » 6.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new revisions.patch5.53 KB

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. :-)

fax8’s picture

fax8 commented
Assigned: fax8 » Unassigned
Category: bug » task
Priority: Critical » Normal
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).

fax8’s picture

fax8 commented
Status: Needs work » Needs review
StatusFileSize
new adding_revisions_support-358605-1-fax8.patch5.9 KB

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

jdipper’s picture

jdipper commented

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

fax8’s picture

fax8 commented

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.

jdipper’s picture

jdipper commented

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

jdipper’s picture

jdipper commented

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

Jack

Crell’s picture

Crell commented
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?

fax8’s picture

fax8 commented

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.

Status: Fixed » Closed (fixed)

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