Wouldn't it make more sense to check the last modified file time before recreating the less.css file, rather than have a developer mode that recreates everything on every single page load?

Something along the lines of if (!file_exists($output_file) || filemtime($file_uri) > filemtime($output_file)) {.

CommentFileSizeAuthor
#7 less-last-modified-with-imports-1169114-7.patch8.06 KBjox
#3 less-last-modified-with-imports-1169114-3.patch7.29 KBjox

Comments

corey.aufang’s picture

corey.aufang commented
Status: Active » Closed (works as designed)

Only problem with checking modified time is that it only checks the file added with drupal_add_css or from the info file.

This precludes rebuilding when changes are made in a @import'ed .less file in another .less file.

patrickharris’s picture

patrickharris commented

True. But you could always check each file for @import files. There's such a huge overhead involved in rebuilding on every page, otherwise.

Something along the lines of ...

      if (file_exists($output_file)) {
        $output_file_time = filemtime($output_file);
        $basepath = dirname($input_file);
        
        // This is too slow for larger files
        #$css = file_get_contents($input_file, FALSE);   
        
        // read line by line
        $css = '';
        $fp = fopen($input_file,'r');
        if ($fp) {
          while(!feof($fp)) {
            $css .= $line = fgets($fp);
            if (strpos($line, '@import')=== false) {
              break;
            }
          }
        }
        fclose($fp);
        
        preg_match_all('/@import\s*(?:url\(\s*)?[\'"]?(?![a-z]+:)([^\'"\()]+)[\'"]?\s*\)?\s*;/', $css, $imports);
        
        foreach($imports[1] as $file) {
          if ($basepath && !file_uri_scheme($file)) {
              $file = $basepath . '/' . $file;
          }
          if (filemtime($file) > $output_file_time) {
            $_recreate = TRUE;
            break;
          }
        }
      }
jox’s picture

jox commented
Status: Closed (works as designed) » Needs review
StatusFileSize
new less-last-modified-with-imports-1169114-3.patch7.29 KB

I was thinking the same and wrote some code that does the following:

  1. When each less file is generated, recursively create a list of all files that get imported and permanently store the list with the name of the main file as key.
  2. When a less file is requested, and exists, check the modify time of each of the files in the respective list. If any is newer than the generated file, regenerate the main less file (and the list of imported files).

Caching the lists of imported files reduces even more overhead.

This is in total much less overhead than regenerating all less files on each request.

A patch against 7.x-2.x-dev is attached.

jox’s picture

jox commented

Some additional notes to the patch:

  • I changed some system messages accordig to the new functionality.
  • A drupal notification message is displayed when a less file is regenerated due to a changed file. This has a problem though. The message will not appear at the page request it is set. It is stored in the session and will not appear until the very next page request. How can this be fixed?
  • A warning message is displayed if an imported file does not exist.
  • The permanent list of imported files for each less file is cleared when less cache is flushed manually.
jox’s picture

jox commented

One more thing needs to be mentioned. There is a bug in core that also needs review #1198904: drupal_load_stylesheet() fails to load @import files in different directories.

The bug might affect css files that import other files that are not in the same directory.

patrickharris’s picture

patrickharris commented

@jox Your patch works well for me - thanks!!
I guess, being a bit anal here, it seems wrong to have a lot of @import files being loaded on each page load (via variable) if they aren't being used - so perhaps you could delete the 'less_imported_files' variable when someone turns off the developer mode?

jox’s picture

jox commented
StatusFileSize
new less-last-modified-with-imports-1169114-7.patch8.06 KB

@patrickharris:
I'm glad it works for you.

I'm not sure if I understand you correctly. What I can say is:
The 'less_imported_files' variable does only store file names (with path). But not the contents of the files.
The variable will never be loaded when developer mode is not set.
The variable will be deleted on less cache flush.

But I agree it would be good to delete the variable when developer mode is turned off, just to clean up.

So here is a new patch with the following changes.

  • Added that the variable 'less_imported_files' is deleted when developer mode is turned off (added less_settings_validate()).
  • Added that the variable 'less_imported_files' is deleted also in _flush_less() (not only in less_flush_caches()).
  • Fixed some system messages (added t()).
  • Fixed non existing files getting registered (and produce errors).
patrickharris’s picture

patrickharris commented

@jox - excellent! I'll try the new patch & report back. Thanks for that.
Re the variable - it is loaded into global $conf on every page request. It's a very small thing, but best to delete it when it's not needed I think.

jox’s picture

jox commented

@patrickharris: Uh, now I get it. You are of course totally right. All the variables are being loaded on every request (into $conf), even if they are not accessed. I didn't consider that. So thanks for the hint.

patrickharris’s picture

patrickharris commented

I hope this patch gets committed!

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

Subscribing. Would also love to see this committed.

bsztreha’s picture

bsztreha commented

Subscribe! Good idea
(i think about other idea: regeneration of files depend on logged USER - designer)

patrickharris’s picture

patrickharris commented

I'm not sure why this has been left neglected? I'd love to see this committed. Is there any reason holding it back? It seems to work fine for me.

xen’s picture

xen commented

Because it's been suggested and implemented before, but the maintainer didn't like it: #789410: Cache cleaning

patrickharris’s picture

patrickharris commented

It's a great pity. I totally agree with your thoughts in that thread.

corey.aufang’s picture

corey.aufang commented

I'm working on a solution that will incorporate the fixes from #1198904: drupal_load_stylesheet() fails to load @import files in different directories as well as checking mod times of nested files. I think that if this module were to include its own version of drupal_load_stylesheet that it make the module more stable.

#1198904 is quite important here since you are more likely to have libraries of LESS and therefore include more files through @import.

It also might be time to work in #988552: add import paths since this is related to having correct paths when navigating through file systems.

jox’s picture

jox commented

@corey.aufang: Sounds good. I only think that including an own version of drupal_load_stylesheet is not a good long term solution. It makes sense for now until it is fixed in core. But then it would create redundant code and also violate Drupals policy of avoiding duplicate efforts.

Otherwise you could copy even more core code to your module to make it even more stable. Not a good approach.

It would be different of course if we'd need a drupal_load_stylesheet with different functionality.

corey.aufang’s picture

corey.aufang commented

There is a solution for this issue by using lessphp v0.3.7+ along with some code changes I'm making to the module.

This will allow for filemtime checking, including all children, and does it all without making our own version of drupal_load_stylesheet().

Might have to require that version for the module to work. Or a major version bump.

corey.aufang’s picture

corey.aufang commented

Please check out the latest beta.

If you are still finding problems, please retag this issue with the new version as all future development for D7 will be on the 7.x-3.x branch.

corey.aufang’s picture

corey.aufang commented
Version: 7.x-2.x-dev » 7.x-3.x-dev
Category: feature » task

Migrating to task as this functionality is now included in the 7.x-3.x version.

Check out the latest 7.x-3.x-dev version to test.

corey.aufang’s picture

corey.aufang commented
Status: Needs review » Fixed

This should be resolved in 7.x-3.0.

Status: Fixed » Closed (fixed)

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