yched pointed out in [#http://drupal.org/node/525622] that we still have $vocabulary->module, even though hook_term_path() was removed with #320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks. It's too late to remove it in D7 without any other particular reason, but let's kill it as early as possible in the D8 cycle.

CommentFileSizeAuthor
#2 646312-remove-vocabulary-module.patch2.81 KBmr.baileys

Comments

catch’s picture

catch
he/him
Primary language English
commented
Priority: Critical » Major

Downgrading all D8 criticals to major per http://drupal.org/node/45111

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Active » Needs review
StatusFileSize
new 646312-remove-vocabulary-module.patch2.81 KB

The patch attached removes {taxonomy_vocabulary}.module from taxonomy_schema, implements an update hook for upgrades, removes the logic pertaining to $vocabulary->module from the taxonomy module and fixes path.test (which used {taxonomy_vocabulary}.module == 'forum' to get the vid for forum).

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community
Issue tags: +schema change

This is not major (let alone critical, what was I on in 2009) but the patch looks great.

I'm marking RTBC but we may want to wait for #1182290: Add boilerplate upgrade path tests before actually committing it.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Postponed
Issue tags: +API change

I'm postponing this on #1182290: Add boilerplate upgrade path tests. We should not commit any patches to 8.x while there is zero test coverage of the upgrade path, and while 6-7 upgrade path still has major/critical bugs.

catch’s picture

catch
he/him
Primary language English
commented
Status: Postponed » Reviewed & tested by the community

That patch is now RTBC.

catch’s picture

catch
he/him
Primary language English
commented

#2: 646312-remove-vocabulary-module.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs review

Is this really a good idea?

If I'm not mistaken, most of our configuration-based/-collated things do have a module "reference", which apparently helps to identify and find stuff pertaining to the module when attempting to uninstall it.

catch’s picture

catch
he/him
Primary language English
commented

For node types we have module, but when using node_type_save() it is set to 'node_content' which is a magic flag meaning does not belong to a module. field_create_field() and other more recent APIs don't use that. User module does for permissions but there's an open issue about assuming a permission only belongs to one module as being broken. And in D6 the only use of module for vocabularies was hook_term_path(), which iirc the only widespread implementation in taxonomy_redirect() was updating the column to point to itself so it's hooks would run. For things generated from info hooks it can (but sometimes doesn't) make sense. I don't think it works for configuration at all.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +API clean-up

Thanks, #8 makes sense.

Though to me it looks like this patch is almost setting a pattern/direction, which would mean to file follow-up issues to remove the module column from other configuration data throughout core. These kind of issues are a bit hard to evaluate, since we don't know whether we're going to need the existing module reference data in the future and once the info is removed, it is irreversible.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

I committed this earlier today but forgot to update the issue status. Committed to 8.x.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

This issue is marked "api change" and "fixed", but it has no API change node. Can someone familiar with the issue please make one? Thanks...

catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs work
Issue tags: -API change +Novice, +Needs change record

We only need to document the schema change. I'm about to go to bed but moving to CNW and adding the tag so this doesn't get lost.

Also tagging as 'novice' in case someone wants to pick it up - http://drupal.org/node/add/changenotice

Should say something like "{vocabulary}.module has been removed in Drupal 8, this table was not used in core since Drupal 6".

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical
Status: Needs work » Active
catch’s picture

catch
he/him
Primary language English
commented
Title: Remove $vocabulary->module » Change notification for: Remove $vocabulary->module
gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Active » Needs review

http://drupal.org/node/1294406

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Fixed

That's plenty.

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Change notification for: Remove $vocabulary->module » Remove $vocabulary->module
Priority: Critical » Normal
Issue tags: -Novice, -Needs change record