The administration view creates a comments link in the menu system even if the comments module is disabled and/or uninstalled. When the module is enabled, the menu item appears correctly, but if the module is disabled, the menu link still appears, but does not have any text in the link.

Comments

sun’s picture

Title: Administration views creates a comments link despite disabled comments module » Comments link despite disabled Comment module
Project: Administration menu » Administration Views
Version: 7.x-3.0-rc1 » 7.x-1.x-dev
Component: User interface » Code
Issue tags: +Needs backport to D6
dawehner’s picture

Status: Active » Needs review
StatusFileSize
new1.25 KB

The idea of this patch is to get the module which "provides" this view. Therefore the schema of the base_table is loaded.

To get this working i changed one base_table of a view, to be sure this is working

sun’s picture

Hm. I actually silently hoped that Views would have some hidden internal feature baked in that reveals the module dependencies of a view... but if that is not the case, then, hm.

We're going to face this issue at least one more time again, with the "Language" column in the node/content view, since that only really exists (or should matter) when Locale module is enabled. That's a more granular case though.

For this case, I wonder whether should perhaps simply rename the default views files? À la MODULE.WHATEVER-PATH-ID.inc - and extract and test for the MODULE part afterwards?

That said, but I definitely don't want to block this issue on that, I also recently started to play with the idea of possible options for contributed modules to plug in to admin_views - by default. I.e., if you have those modules installed, and you install admin_views, then you get the whole magic for free (whereas magic == default views using the System display handler). If not, those modules provide their own administrative UI, as usual. -- What I'm essentially after in the end is some sort of an algorithm to figure out module dependencies for (administrative, module-provided) default views, I guess...

sun’s picture

Status: Needs review » Reviewed & tested by the community

The drupal_get_schema() looks a bit scary in terms of performance, but I guess/hope that hook_views_default_views() is only invoked rarely.

This won't resolve the optional "Language" field/column issue I mentioned in #3, but that might be a separate issue anyway.

Thus, I'd be fine to go ahead with this as a stop-gap fix.

dawehner’s picture

Yeah hook_views_default_views is only executed when you flush the caches.

We're going to face this issue at least one more time again, with the "Language" column in the node/content view, since that only really exists (or should matter) when Locale module is enabled. That's a more granular case though.

This should actually work already, as long you don't get confused by "broken field" in the admin ui.

For this case, I wonder whether should perhaps simply rename the default views files? À la MODULE.WHATEVER-PATH-ID.inc - and extract and test for the MODULE part afterwards?

This will work.

sun’s picture

Title: Comments link despite disabled Comment module » Comments view link appears despite disabled Comment module
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new2.79 KB

First of all, I've committed @dawehner's patch in #2.

Attached patch is what I suggest.

sun’s picture

StatusFileSize
new2.81 KB

Sorry, missed a DRUPAL_ROOT in there.

sun’s picture

StatusFileSize
new2.78 KB

And meh, the $paths are entirely obsolete.

sun’s picture

Title: Comments view link appears despite disabled Comment module » Default views appear despite missing module dependency; no way for other modules to provide default admin views
Status: Needs review » Fixed

Totally works. Thus, thanks for reporting, reviewing, and testing! Committed to all branches. (backported accordingly)

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

dimitriseng’s picture

We're going to face this issue at least one more time again, with the "Language" column in the node/content view, since that only really exists (or should matter) when Locale module is enabled. That's a more granular case though.

This won't resolve the optional "Language" field/column issue I mentioned in #3, but that might be a separate issue anyway.

I am using the latest dev, but the Language field/column is not included in the node/content view. Are there plans to include it or a separate issue needs to be created for this? Thank you.

sun’s picture

Status: Fixed » Active

Indeed, I did not have Locale module enabled when I re-exported the views. :-/

sun’s picture

Note: I've enabled automated testing for admin_views now.

Thus, when restoring the Language field/filter, let's also add an automated AdminViewsLanguageTestCase test that ensures we don't introduce this regression again in the future.

dimitriseng’s picture

@sun, thanks for the clarification. Is this just a case of you reexporting the views with Local enabled or is there more work involved to get Language field/filter availabe? This is quite useful for multillingual sites. Thank you for your great work.

sun’s picture

I briefly scanned the commit history and apparently, the Language fields/filters seem to have gone lost much earlier in history. I wasn't able to locate when exactly.

If I understood @dawehner correctly, then we should be able to include the Language field/filter in the exported default views, but should naturally expect a Views notice about "broken handlers" on sites which don't have Locale module enabled, but which can be ignored.

FYI: This seems to be last issue that blocks a stable D7 release.

damiankloip’s picture

@sun, what is going on with this? Is there anything we need to do? If so, let me know.

sun’s picture

Status: Active » Needs review
StatusFileSize
new12.84 KB

Views currently doesn't expose the Language field/filter when only Locale module is enabled. Created a patch for that:
#343178: Node language should be available without node translation being enabled

With that out of the way, the "separate display" approach would look like attached patch.

However, I'm not happy with having to override the entire field + filter configuration, when all we want is to add a single field + filter.

sun’s picture

Status: Needs review » Needs work
StatusFileSize
new1.66 KB

Also, #16 blows up when Locale module is not installed.

This would be an alternative approach, but:

1) requires custom code

2) doesn't work, because "locale" alphabetically comes before "node"

So in turn, this pretty much leaves us with introducing a completely custom hook_admin_views_default_views_alter() in order to support this case.

Or... we just ignore this entirely.

damiankloip’s picture

I would be tempted to not include this functionality at all, I don't think it's the most common use case either. I think more people won't use this than will (I might be wrong on that though). I know we don't want to go down the route of custom code but if we did could we not use hook_views_default_views_alter to do what we needed (briefly discussed in IRC)? We could also do this directly in the exported view include, but this would make it more cumbersome when re exporting.

dimitriseng’s picture

+1 for inlcuding the language filter/field in the node/content view.

I understand the technical complications of including this functionality, but I believe that this is very important for multilingual sites, if the language filter is not included then the node/content view would become unusable for these sites, and I am sure that these are a lot, so I hope that a way will be found to get this added. Many thanks for your great work.

sun’s picture

Status: Needs work » Fixed

All of the above explored approaches look like a pain to maintain to me, and I'd totally place a bet that we'd forget to update one of the overridden views or displays or alter hook code in future exports.

It seems more reasonable to wait until Views supports "conditional plugins" so we're able to (natively) limit the addition of the Language field + filter in the exported views definition to when Locale module is installed.

I'm not sure whether there is a feature request against Views for that already, but if not, we should totally create one ;)

(And btw, the problem isn't unique to admin_views — I know that the exported views for drupal.org also contain custom edits in order to handle conditional definitions based on whether module XYZ is available or not.)

damiankloip’s picture

I think this is the right decision :) Removing tags.

dimitriseng’s picture

I guess also that this is the right decision based on the comments above. However, this makes this module almost unusable for multilingual sites, so I would suggest that somebody who understands what is required to get this working, submits a feature request for Views as suggested by sun in #20, if one does not exist already. Thank you.

damiankloip’s picture

dimitriseng: If you have a multilingual site, you can just add this filter to your admin views and override them, it will be no bother at all really, and will only take about 10 seconds.

Status: Fixed » Closed (fixed)

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