Posted by eaton on July 14, 2006 at 3:54am
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | path.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | needs work |
Issue Summary
A number of module tables, like taxonomy, don't need to be in system.install. This bundle of install files splits them out properly. It's a zip file because I have no cvs-fu, and don't know how to make a patch that adds files to core. ;)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| install_patch.zip | 34.24 KB | Ignored: Check issue status. | None | None |
Comments
#1
Added $Id$ and rolled a patch. Thanks Eaton!
#2
Works smashingly, though it does depend on http://drupal.org/node/73615 (a patch htat always forces system.module to install first, no matter what order things appear in).
Without #73615, menu.module tries to install first, before the variables table exists, etc.
So. Commit #73660, then this one. And the installer will be happy.
#3
This is CVS, clearly not 4.7.2
#4
#73615 (dependency) was committed. I'm cool with this patch.
#5
I have always wanted this, but it's a little late
#6
I have always wanted this, but it's a little late
#7
Now it's too late again... let's get it done in D7!
#8
These are the tables that are created in system.install
- variable
- actions
- actions_aid
- batch
- blocked_ips
- cache
- cache_form
- cache_page
- cache_menu
- cache_registry
- files
- flood
- history
- menu_router
- menu_links
- registry
- registry_file
- sessions
- system
- url_alias
Which are save to move to their respective modules?
#9
I'm thinking the only others that can be split is possibly url_alias. Everything else is needed by system.
Patch attached to split url_alias schema into path.install. Tested on fresh HEAD install and upgraded HEAD install.
#10
Since there's only one module left to be split out from system.install, renaming the issue. Revised patch with the fetchField() from DBTNG.
#11
The last submitted patch failed testing.
#12
Re-roll.
#13
the $paths_enabled check should be a module_exists('path') - this was tried a while back but had to be reverted because module_list was broken, I think it's less broken now so it should be fine to do now. That'd then save us the COUNT() query altogether since whether path module is enabled is a sufficient check in itself.
#14
Yeah I suppose that would be a better check. Easy enough of a fix!
#15
We can still kill the COUNT() though. I realise this has nothing to do with the purpose of the patch, so leaving at CNR.
#16
The last submitted patch failed testing.
#17
For some reason, module_exists does not work there. It returns FALSE even though the module is enabled. Switching back to db_table_exists and removing the $count stuff.
#18
hmm, module_list() is still broken then. You left a commented module_exists() check in the patch though. I think this means #12 is RTBC anyway. Sorry for derailment.
#19
#12 no longer applies but #17 applies (with minor fuzz) and installs correctly, path.module enables correctly but trying to uninstall path.module throws an exception:
#20
Ah watchdog had this to offer:
#21
#22
#23
Gave this a re-roll now that module_exists() looks like it works within drupal_lookup_path(). I tried uninstalling the module and it worked successfully.
#24
I know it's scope creep, but if module_exists() actually works at that stage, can we pretty please kill the SELECT COUNT(*) in drupal_lookup_path()? If not this at least gets me subscribed so I can revive the old patches which did this.
Everything looks good, so if the test bot says yes, ought to be RTBC.
#25
I guess it's reasonable to assume that we can remove the count query since if the path module is installed, then it's assumed that the user is actually using some url aliases.
#26
There's a small chance that someone has path installed and no aliases, then they'll get lots of no-op queries, but there's a guarantee that every other Drupal site gets the count query on every page request, so overall, it'll be less ;)
#27
Revised patch just using module_exists().
#28
The last submitted patch failed testing.
#29
I'm not sure this patch makes sense because it ties path.inc (an API for having URL aliases) to path.module (one particular user interface for managing them). See the discussion at #196862: COUNT(*) is an expensive query in InnoDB. also.
#30
The function drupal_lookup_path has always been tied to path.module. The {url_alias=} table should have always been tied to the path.module as well and allowed it to be uninstalled. Nothing is being changed in that respect. It's only allowing path.module to be uninstalled.
#31
Looking at the current code for drupal_lookup_path(), I'm having trouble seeing how it is tied to path.module..... in fact, if I disable path.module on existing site, the drupal_lookup_path() function continues to get called, and my previous URL aliases continue to work. To me, that's the expected behavior.
Consider this scenario: Someone wants to write a contrib module that provides a totally different user interface for managing URL aliases. Instead of the long admin screen and the collapsible fieldset on nodes provided by path.module, they want to do it completely differently. Currently, this would work fine -- someone who wants to try out the new module would just disable the core Path module, install the new one, and everything would work OK. However, if this patch goes in, how would they do it? If they disable the core Path module, their URL aliases would all be broken in the meantime. And if they made the mistake of uninstalling the core Path module, then all their URL aliases would be gone forever and they'd have to start completely from scratch.
I kinda do see the utility of a "delete all URL aliases on my site" kind of functionality, but I just don't think it's something that should happen automatically when the Path module is uninstalled. Not the way the Path module currently works, at any rate.
#32
I guess in that scenario, the contrib module might want to provide a different data structure and load it's own 'path_include' variable. I was always in the assumption that if the path module was disabled/uninstalled, then node aliases should not work.
I guess we should get more opinions on which direction to take as far as what should happen when path is disabled/uninstalled.
#33
Subscribing.
#34
Too late for D7. Bumping to D8.