Set module weight for Modernizr module

Which other module depends on Modernizr and how does it load Modernizr?

I realize this is closed, but I was having this issue as well. The only way I could find to fix it is to use drupal_add_js() to add modernizr if it's not there, in a hook_js_alter() function.

I ran into the same issue also. Here's my workaround, similar to #7:

/**
 * Implements hook_js_alter().
 */
function mymodulename_js_alter(&$javascript) {
  $modernizr_found = FALSE;
  foreach ($javascript as $key => $script) {
    if (strpos($key, 'modernizr') !== FALSE) {
      $modernizr_found = TRUE;
    }
  }
  if (!$modernizr_found) {
    drupal_add_js(libraries_get_path('modernizr') . '/modernizr-min.js');
  }
}

I am also experiencing this issue. I am using this module:

https://www.drupal.org/project/modernizr

After installing Mobile Friendly Navigation Toolbar, Modernizr is not loading for anonymous users and causes my site to break on IE8 and below.

Marked #2316323: Integrate navbar with modernizr drupal module as duplicate of this issue.

I think this edit puts us on the correct path.

So it turns out we were already halfway there. As described in the modernizer documentation, contrib modules simply need to define their required tests in hook_modernizr_info - which navbar already does. This patch removes the code that adds the modernizr library itself. I've also left in the library weight definitions that ensure that modernizr loads before backbone and underscore. I've tested with the Aurora theme and everything works as expected.

The documentation will need updating to point people to the modernizr module and to make sure that everyone understands that they must download a new production build by clicking button on the modernizr settings page in order to include the navbar tests.

Thank you, the patch in #12 brings sanity to navbar's handling of the modernizr library. Tested and works as expected, navbar uses the modernizr module loading of the library rather than its own methods. Tested with Omega 4.x based themes.

Thanks for testing. This is a pretty major change that will definitely break for users who upgrade without following the instructions. Should a 2.x branch be started to integrate this change? How are backward-incompatible changes like this normally handled?

A 2.x branch could also move to using the backbone module and making both the backbone and modernizr projects dependencies so that navbar doesn't need any custom library installation instructions. I haven't reviewed that particular backbone implementation but it seems to have a similar library installation process to navbar.

Agreed @Fidelix. It would be nice to see a new "library-only module" for backbone though. A backport of https://www.drupal.org/node/1149866 might be an option. We can open a new issue if people are interested enough...

I am getting modernizr is not installed error on my status logs.

If the modernizer module is enabled, shouldn't navbar check via the modernizr module if it is enabled and use modernizr module library functions to check if the file is present in the library folder?

Applied patch from #12 on 7.x-1.4 successfully, everything peachy.

Upgraded to modernizr sept 22 dev, upated navbar and it seems to work now.

i do not like installing full modernizer module only for navbar. The lib is very large where the required code here is very small. I suspect this code removal break every site without the moderizer module.

This issue exists because navbar conflicts with other modules that use the modernizr library. The best practice is to use a stand-alone library for third-party tools that all contrib modules can use. The module is required because we don't want to include every feature of modernizr but only the ones that modules need to reduce code.

navbar's modernizr.js = 3.6K
stand-alone library modernizr.js = 7.7K
combined mordernizr.js after applying this patch = 7.4k

I addressed the breaking of every site in comment #14.

Sorry, but You remove all lib checks. This module does not require modernizr. This patch is against 1.x and not 2.x. It doesn't matter if this solves issues for a minority modernizr module users and on the other side it breaks all lib checks for all other users. If there is a problem, ok - but it need to be fixed for all users, not the minority of modernizr users only. I do not see that this patch adds the unwished requirement to the modernizr module and I guess This code removal will only break things. There are reasons why this code has been added in past...

1. The lib checks would fall to the modernizr module. A dependency of the modernizr module will need to be added to navbar. I'm leaving this in needs work to get that done - thanks for catching that.

2. This module does require modernizr. Navbar shows an error on the status page if it is not installed.

3. I don't have the ability to create a 2.x branch. I already asked the question "Should a 2.x branch be started to integrate this change?". Maybe you could help push for that if you believe it's a good idea too.

This solution is meant to work towards your point that "it need to be fixed for all users, not the minority of modernizr users". The modernizr module's API is built to solve that need. I don't see another way of limiting the feature-set in a consistent way between contrib modules.

#2139345: Include Modernizr dependencies in hook_modernizr_info()

I do not know why there is a compatibility problem, but fixing it looks like a lot better idea than forcing a integrating with these modernizr module.

If you don't understand why this is a compatibility problem then stop hijacking this issue. If you don't want to contribute anything constructive then walk away. This patch is fixing an issue for several people already.

You confirmed that this changes breaks the module for every non modernizr module user and it requires a dependency on modernizr module. The first is not acceptable and for the second a decission was already made several months ago - this module should not depend on modernizr module. See the duplicate case for jesses decission. Try to fix the bug you expierence, but do not make the module depend on modernizr module, please.

I've added a support request to figure out whether or not modernizr is required. If it is not required then adding a dependency definitely doesn't make sense.

https://www.drupal.org/node/2351371

@hass, I am considering adding a hard dependency on Libraries 2.0 which would drastically reduce the size of the Modernizr module codebase. Would that make a second dependency more bearable? You already have the Libraries dependency.

Or perhaps making a very small submodule inside of navbar to encapsulate all the Modernizr stuff would be possible? Then the dependency is limited to people who actively enable the Modernizr-related functionality.

I'd suggest the latter. I have to use modernizr module and having modernir loaded twice gives unpredictable errors.

The main questions that comes in mind here for me - are the library hooks used/implemented in a wrong way in navbar or modernizr module? Or are we victims of a libraries module bug? We shoulf first focus on this and once this can be answered we can start fixing in the right place. Just removing some code without understanding the root cause sounds wrong to me. Let's identify the root cause first. I have not read any bug analysis in this case yet. I think it need to be possible to define library hooks in both modules without a conflict.

I hope the root cause is not just a trivial issue like a module weight only as an example.

What is the current state of this issue? I still have troubles with Modernizr module version 7.x-3.3 (Modernizr v2.8.2) and Navbar module version 7.x-1.4. I also use a Aurora subtheme, generated with Aurora 7.x-3.4.

Does the current Modernizr dev-version 7.x-3.x-dev address this issue?

I ran into a similar issue with OpenideaL, a distribution which uses Navbar. In short it appears that Navbar was blocking access to modernizr.js. Initially, the only fix was to disable Navbar.

Solution:

• navbar-7.x-1.4 + patch in #12
• modernizr-7.x-3.3

thanks!

Thx bisonbleu, applying patch #12 worked for me too with my above mentioned setup.

The current state of this issue is that there is resistance to using the modernizr module as a dependency. The point of the modernizr module is provide a sane, consistent way for contrib modules to "share" the library and generate a modernizr installer that combines only the features that are required by each contrib. IMHO this makes sense and is in the spirit of the community. I've been met with resistance from one person so rather than argue I'm waiting on more feedback/support before investing more time into this.

No idea why nobody has an answer to #34.

As far as I'm concerned, it's a matter of context and perspective.

My installation requires modernizr (or parts of it) in at least 2 separate situations: navbar & an image grid that needs to be fully responsive. In the Open IdeaL ditribution, modernizr.js can be found at /profiles/idea/libraries/modernizr/modernizr.js. But for some unknown reason modernizr is not loaded; this is confirmed on the Satus Report page. Installing a second copy of modernizr at sites/all/libraries changes nothing. So I'm stuck.

Now, if I'm being practical (context), I have 2 short term solutions:
- Disable navbar to allow modernizr to be properly loaded.
- Leave navbar enabled, apply patch in #12 and install the modernizr module

Like most of us, I need something that works now - even if it's not perfect or ideal.

I mostly agree with what @hass is saying in #34. It does makes sense to investigate the implementation of the library hooks as well as the libraries module in search of the root cause of this issue. Any endeavour that makes navbar better is a good endeavour.

I also agree with what @mducharme and others are hinting at: let the modernizr module 'manage' modernizr.js for all modules that require it. I'm not an expert but it does seem to make sense. And wouldn't that standardized things a little? Assuming it does, then that sounds like a good ROI.

Summing up, both threads/contexts are legitimate. The challenge, IMHO, is to make them work in the same perspective.

Good summary. Identifying the root cause as outlined in #34 is the first step and why this is still "needs work". The patch is just a temporary fix and should not be committed since it will break existing installs as I detailed in #14.

I am switching this back to active. It isn't in "needs work" because there isn't any idea as to what needs to be done.

I don't think that this module should depend on modernzr because the use case for modernzr is custom builds of the library. This module should be able to play nice with any other module that is defining a library by the same name. If there is a conflict then it is likely because something is wrong in modernzr, navbar, or libraries.

My (absolute) guess is the issue lies with modernzr. Modernzr is built around the concept that one would use a custom build of the lib. It should then use namespacing or another means to protect the lib that it is being used with.

CNW because there is a invalid patch that was RTBC.

Tested with patch #12 on 1.4... Thanks, works as expected

Haven't had a chance to fully test this, but at a minimum, the status report error goes away and my site still works.

Line 773 of navbar.module (7.x-1.5) reads:

'file' => 'modernizr-min.js',

Changing that line to read:

'file' => 'modernizr.min.js',

The modernizr module installation states:
Copy modernizr-X.Y.min.js to sites/all/libraries/modernizr/modernizr.min.js

Don't know if this was a change from past versions as I just started using it.

Seems to have worked on my end. If I come across any errors, I'll let you know.

I also noticed this issue and completely forgot to mention. I changed line 773 in the same way @PDonaghe did and everything seems fine. On second thoughts however maybe it should be the library that's renamed to -min.js as that would fall in line with underscore etc

I also changed line 796 in a similar way

I think the issue is as follows. navbar defines a library with name modernizr. This name conflicts with modernizr module and maybe because of module weight reasons this get's overridden by the navbar module. I think we are doing something wrong in navbar module.

Navbar module should not define a library with name modernizr. It should define a library name modernizr.navbar. As we require a very special build for navbar it should also have a different name in the file system. I propose something like modernizr.navbar.js and modernizr.navbar.min.js. This way we could add both files sites/all/libraries/modernizr/modernizr.min.js (the full version from modernizr module) and the navbar version to sites/all/libraries/modernizr/modernizr.navbar.min.js. In case the modernizr module is enabled navbar does not require the navbar version.

I think this way we could allow users to use one or the other.

I also tend to say we should add modernizr.navbar.[min].js files to the navbar module itself. The build process is so complicated... this is really usability nightmare and I do not think any other module should use modernizr.navbar library.

This is just brainstorming, I haven't tested this yet.

I agree with Hass' suggestion of including Modernizr in navbar. The Modernizr lib doesn't really have a good way to integrate so I wouldn't suggest we add the Modernizr module as a dependancy.

If we do not include the js lib in the navbar project then I would suggest we change the library directory. sites/*/libraries/modernizr_navbar/modernizr.navbar.min.js
We can use modernizr_navbar as the libraries api info name as well. This should stop any conflict.

Any thoughts?

Why do you like to add a new library folder?

To leave the modernizr directory alone for the modernizr module. It can be complicated to share the directory with two js libraries. This is made more complicated if someone wants to use npm to install the modernizr lib and grunt to build it.

Making Navbar depend on the Modernizr module is a reasonable move, IMO.

Duplicating the library for every module that might need it, however, is a silly suggestion - especially when the Modernizr module solves this exact problem.

At the VERY least, checking if the Modernizr module is installed before defining our own library seems like a perfectly reasonable solution. This module already implements hook_modernizr_info so it's already aware of the Modernizr module even if it doesn't have a hard dependency on it. See attached patch.

Have you tested if this is a module weight issue or could be fixed by Modernizr module by using a different weight? Looks like lib name and folder name are still the old.

Have you tested if this is a module weight issue or could be fixed by Modernizr module by using a different weight?

No, and I don't think telling people to go into the database and screw with module weights is a solution - especially since this works fine.

Like I said - this module is already aware of the Modernizr module and implements one of its hooks. Simply avoiding re-implementing the library if the Modernizr module is in use seems like the simplest, least disruptive solution.

No, and I don't think telling people to go into the database and screw with module weights is a solution - especially since this works fine.

Nobody said this. It is something the modernizr module can do with hook_install and hook_update. I just try to find a clean and reliable solution that works all times. The module check may work, but is not that clean like a module weight if this can solve the issue, too.

I think what it boils down to is this: What is Navbar's job and what is Modernizr's job? Then the next natural question is why is Navbar trying to do a piece of Modernizr's job? There's clearly a need for Modernizr outside of Navbar - asking Modernizr to play module weight bingo with any other module that wants to use Modernizr defeats the entire purpose of the Modernizr module.

I still believe Modernizr should be a dependency of Navbar. However, this also works and resolves the conflict. Again, this isn't a problem with Modernizr - it's doing the job it's supposed to do. This is a problem with Navbar.

The root cause is that navbar is not shown for anonymous users (and maybe other roles) and therefore the navbar library dependencies no longer require the modernizr libary what removes the modernizr library from the page. As navbar has a higher module weight it currently wins and kicks out the modernizr library definition.

This patch should fix the issue in general manner for all times and for all upcomming conflicts like these.

I still believe Modernizr should be a dependency of Navbar.

I'm happy that it does not. I have no need for Modernizr at all and like to save download size of this library.

Tested this and everything updated as expected. Hopefully this resolves the conflict between the two modules.

@hass Thank you for your patience in focusing for the root cause while expressing a different view on using specific modernizr scripts for navbar rather than using the modernizr module. A useful discussion. Thanks.