I'm using the rather wonderful grunt-modernizr package for Grunt.js to automatically determine my modernizr custom build requirements *and* download my custom build for me. I have this all working nicely, with the custom build being placed in libraries/modernizr for your module to then use.

The bugger is, the downloaded custom build contains no version number as your module expects it (example attached as .txt). This results in Status report reporting that Modernizr is not installed.

If you search within the attached file for "2.7.1" you'll notice that there is a version number included, albeit a bit more obscurely than any of us would like.

2 questions:

1) Does this prevent the Modernizr module from using the library completely or is it just a minor reporting issue?

2) Can you propose way to fix this?

CommentFileSizeAuthor
#14 2222253-14-regex.patch880 bytesrupl
modernizr.min_.js_.txt9.31 KBdddbbb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dddbbb’s picture

1) It seems this does prevent the module from using the library at all which is frustrating.

rupl’s picture

Version: 7.x-3.1 » 7.x-3.x-dev

Thanks for the bug report! Also thanks for including your whole Modernizr file, it helped me reproduce the issue and start debugging.

Yes, the reporting error seems to be just a surface issue. However at the same time this does seem to be preventing its inclusion on the page, which is bizarre and frustrating to me too.

I thought this would be something simple but unfortunately it is evading me at the moment. Can you just add the version number into your file for now? I downloaded your file and when I manually added the version number it immediately starts working. I'll work on a fix in the meantime, but I can't figure it out right now.

rupl’s picture

Status: Active » Needs review
FileSize
947 bytes

Can you give this patch a try to see if the reporting error is fixed?

dddbbb’s picture

Yes, I'd already reached the same conclusion about manually adding the version number for the time being. That seems to do the trick for now while we figure out a better fix.

I'm also attempting to tackle this from the other end:

https://github.com/Modernizr/grunt-modernizr/issues/75

Either way, I still think that the Modernizr module should not fail to load the entire lib just because of a missing version number.

Using r="([\d\.]*)" on my attached custom build seems to get me part of the way there but my RegX & PHP just aren't good enough to give you a patch worth submitting.

dddbbb’s picture

Right-O. Will give the patch a whirl and report back...

dddbbb’s picture

Oops, didn't mean to delete the patch.

dddbbb’s picture

Yes, the reporting error is fixed with the patch in #3 but the library still fails to load if the version number isn't in the comment at the top of the custom build.

dddbbb’s picture

Status: Needs review » Needs work
rupl’s picture

Status: Needs work » Active

my beautiful patch!!!! haha just kidding.

I agree the module is doing us wrong at the moment. It's a bug, it happens. My PHP is none too sharp these days either. But this is a show-stopper for fully integrating grunt-modernizr functionality as a Drush command so I want this fixed as bad as you do. We'll figure something out.

I committed the reporting error fix to dev. Thanks for testing.

dddbbb’s picture

Status: Active » Needs work

The following pattern is probably a little more robust for singling out the version number from the code (assuming that the variable name "r" is consistent):

var r="([\d\.]*)"

In my attached custom build this will return var r="2.7.1" - we'd then just need to create a sub string and strip off the surrounding cruft.

Could we attempt to grab the version number from the commented code first (as the module currently does) but then if it fails, try to get the version number from the JavaScript (along the lines of the RegX I've quoted above)? Then if both attempts fail, print "Version unknown" in the Status report and don't load the library. Thoughts?

dddbbb’s picture

Status: Needs work » Active

Didn't mean to change status. We keep posting at the same time!

dddbbb’s picture

I've just chanced upon grunt-text-replace and now have a basic grunt task for getting around this version number business in the short term.

Once you have grunt-text-replace installed, add the following task to your Gruntfile.js (make sure it's run after grunt-modernizr):

replace: {
      modernizr_build_version_no: {
        src: ['../../libraries/modernizr/modernizr.custom.js'],
        overwrite: true, // overwrite matched source files
        replacements: [{
          from: 'Modernizr (Custom Build)',
          to: 'Modernizr 6.6.6 (Custom Build)'
        }]
      }
    }

That does a rather slap dash job of adding in a totally made up version number to the modernizr build created by grunt-modernizr just to keep this module happy in the meantime. Obviously you'll want to tweak the path to your modernizr build to match your setup.

rupl’s picture

Clever, thanks for the update! I still can't figure out why this is happening inside the module.

rupl’s picture

Status: Active » Needs review
FileSize
880 bytes

ok, I think I got this! It's a tiny patch but I was able to use your Modernizr build in comment 1 in the OP and it both reports the correct version and successfully prints the <script> tag.

rupl’s picture

BTW, could you share your thoughts on integrating with grunt-modernizr in this issue? #2239291: Integrate with grunt-modernizr

  • rupl committed ce8fede on 7.x-3.x
    Issue #2222253 by rupl, danbohea: fix regex for Modernizr version to...
rupl’s picture

Status: Needs review » Fixed

I made some other decisions based on news from the yepnope team that they have deprecated their code. I'll keep the discussion in the other thread: #2311603: Address policy on upstream support

However, this means we can be a little more strict with our regex and I am just hardcoding it to look for a Semver starting with major version 2 (2.x.y). As an aside, that uglified variable name (r="") seems to change depending on the build, so just checking for the number is more reliable.

Sooo.. I have committed a fix to dev. Thanks again for reporting this and helping me brainstorm it.

dddbbb’s picture

Excellent news - glad I could help :)

Status: Fixed » Closed (fixed)

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