Being required is cool. field_disable(), field_uninstall() can go, and also field_enable(), which was just there to refresh info since the last time the module got disabled :-p
Same goes for field_sql_storage_uninstall()

On a related note :
I noticed that all required modules (filter, node, user) have their schema created by system_install() rather than in their own hook_install(). If that's a pattern, should field and field_storage_sql do the same ? (not included in the current patch)

CommentFileSizeAuthor
#3 field_install.patch1.36 KBchx
field_install.patch2.14 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

yched CreditAttribution: yched commented
Title: Unneeded functions in field_install » Unneeded functions in field.install

Typo in the title

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Bye bye code.

chx’s picture

chx CreditAttribution: chx commented
FileSize
field_install.patch1.36 KB

let's not touch field_sql_storage. Requiring it is a hack until handlers in place.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed #3 to HEAD. Thanks!

yched’s picture

yched CreditAttribution: yched commented
Title: Unneeded functions in field.install » field.install cleanup
Status: Fixed » Active

Reopening for :
"currently all required modules (filter, node, user) have their schema created by system_install() rather than in their own hook_install(). If that's a pattern, should field.module do the same ?"

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Active » Fixed

Node has its schema in node.install: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/node/node.install?...
Same with User: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/user/user.install?...
And with Filter: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/filter/filter.inst...

So I'm a bit confused as to the question?

The only tables that should be defined in system.install are those that are required by .inc files, regardless of what modules are enabled. So Actions, Menu, Path, etc. fall under this. Anything else that's dealt with in purely a module (whether it's required or not) should be in that module's .install file to match what we do in contrib.

Tentatively marking back to fixed, but please re-open if there are still unanswered questions.

yched’s picture

yched CreditAttribution: yched commented

The schema *definitions* live in each module's hook_schema() alright, but the tables are not *created* in hook_install() like for regular modules. node has no node_install(), user has no user_install(), filter etc...
system_install() does: 

  $modules = array('system', 'filter', 'user', 'node');
  foreach ($modules as $module) {
    drupal_install_schema($module);
  }

So my question is :
- is that a coincidence that it happens to be the case just for the modules are required ? (something whispers 'no')
- If it is deemed important that required modules have their tables created before any other module or even themselves are installed, then maybe field should follow that rule too ?

yched’s picture

yched CreditAttribution: yched commented
Status: Fixed » Active

and reopening ;-)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

OH! Sorry! :) I misunderstood!

I actually am not aware of the reasoning behind those lines of code, but I agree that putting field there as well probably makes a lot of sense, for consistency.

Island Usurper’s picture

Island Usurper CreditAttribution: Island Usurper commented
Status: Active » Closed (fixed)

Oh, hello, year-old issue! #306151: Automatically install/uninstall schema obsoletes this, since system_install() doesn't create tables any more (except its own).