This is coming in light of #691932: Add hook_field_schema_alter() having no future (should have been filed sooner so we could not have it in D8 but anyways). There is a clear inconsistency in how we allow contributed modules to treat field table schemas vs. table schemas from other modules. There are a number of reasons mentioned in https://drupal.org/node/691932#comment-2537356 which, really speaking, would also apply to non-field tables. So the easier solution is to just remove the table altering capabilities.
Comment | File | Size | Author |
---|---|---|---|
#10 | 2060629.d8.patch | 4.64 KB | BTMash |
#9 | 2060629.d8.patch | 2.77 KB | BTMash |
#1 | 2060629.d8.patch | 2.78 KB | BTMash |
Comments
Comment #1
BTMash CreditAttribution: BTMash commentedAnd I've attached the patch (surprise surprise there were no tests for hook_schema_alter in the first place so there was very little to remove).
Comment #2
Dave ReidJust pointing out we need an alterative to this, since this would screw modules like File entity which *must* alter the entity base table since we're adding a bundle column.
Comment #3
BTMash CreditAttribution: BTMash commentedI'm not exactly sure what the alternative would be, seeing as we are dealing with having alterable schemas or not having alterable schemas (ie. the rules should be across the board). Regardless, wouldn't this be a case for the files table in core to have a bundle column in the first place?
Comment #4
BTMash CreditAttribution: BTMash commentedFor now, marking this as 8.x atleast to make sure the tests don't fail.
Comment #5
BTMash CreditAttribution: BTMash commentedNow that it shows the patch passes, back to 9.x
Comment #6
BTMash CreditAttribution: BTMash commented1: 2060629.d8.patch queued for re-testing.
Comment #7
BTMash CreditAttribution: BTMash commentedSuddenly dawned on me that this really shouldn't be in 8.x. There are no tests in core for this.
Comment #9
BTMash CreditAttribution: BTMash commentedRerolling
Comment #10
BTMash CreditAttribution: BTMash commentedRealized there is a schema.api.php that needs to be adjusted as well.
Comment #11
sunComment #12
Crell CreditAttribution: Crell commentedI've never liked this hook. :-) Bye bye, hook.
Comment #13
Dave ReidI still have yet to have a good answer about how modules like File entity in D8 will be able to provide bundles for file entities without altering a core table's schema. I don't agree with RTBC at all.
Comment #14
sunTo address that concern, I think we need to get back to #1295148: Maintain common properties/statistics about entities and consider to bump it to critical. I just outlined some thoughts over there.
Comment #15
Crell CreditAttribution: Crell commentedIMO, the answer is "make Files bundleable in core because needing a separate module to make them useful is stupid." It's a bug, let's fix it.
Comment #16
Dave ReidThen don't forget users aren't bundleable too! Because they're in the same situation as files. Oh wait, we want to actually get Drupal 8 out before 2015?
Comment #17
sunFixing some slightly weird issue properties here.
Comment #18
webchickI don't quite understand how #1295148: Maintain common properties/statistics about entities addresses Dave Reid's concern, but it seems to me that this issue should be postponed on either that or a different issue to resolve #15/#16.
Comment #19
webchickHere's a list of "all" (as of a couple of months ago) modules that implement hook_schema_alter(), btw:
There may be a few false-positives in that list, but it's still quite significant.
Given that, I don't see how we can possibly just remove this hook willy-nilly. We need to ensure core supports those modules' use cases before we can commit something like this. Postponing on whatever sub-issues are needed to make that happen.
Comment #20
BTMash CreditAttribution: BTMash commentedAdding the catalyst relationship to the issue - if field schemas cannot be altered, then regular module schemas should not be alterable (as same sets of concerns apply). If regular module schemas can be altered, then field schemas should be alterable.
Comment #21
Dave ReidAm I the only one that thinks that altering normal schema is fine and not being able to alter field schema is fine as well? Stuff in hook_schema() I know is a database table. Stuff in hook_field_schema() I am not sure where it will live (but could be database), so I can assume I cannot alter it. I'm absolutely fine with that assumption and see no need why we can't just live with that documented?
Comment #22
Crell CreditAttribution: Crell commentedA design goal of Drupal 8 is no assumption of SQL. Modules should not be altering table definitions willy nilly because core makes absolutely no promise whatsoever that it's using an SQL database, from any vendor. Modules that hard-code themselves to SQL are, IMO, "doing it wrong".
All entity storage could be in non-SQL. Most config is now CMI, which is YAML. State is now the State API, which is key/value and could (probably should) be Mongo or Redis, not SQL, if available. ctools-ish cases are Config Entities (CMI (YAML)). Queue has always supported non-SQL. Even Batch API is, via some trickery, not SQL-dependent. I don't think there are any systems remaining that you can rely on being SQL, and that's by design.
Dave: I've long argued that bundle-able and non-bundle-able entities were a stupid split to make, so you don't need to work very hard to convince me that users should be bundleable. :-)
Comment #23
Dave ReidWhy do we even have hook_schema() then? It naturally assumes SQL.
Comment #24
Crell CreditAttribution: Crell commentedHonestly, that's a question I've asked myself as well. See also: #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions
Comment #25
catchSo there's two reasons to use hook_schema_alter():
* Modify the behaviour of drupal_write_record() and drupal_schema_field_sql().
* Document schema modifcations that aren't anything to do with dwr() dsfs(), just so that your site still validates with https://drupal.org/project/schema
In both cases to modify the actual database schema, you have to call db_*() functions directly to make those changes, hook_schema_alter() is not called when a module's schema actually gets installed.
For dwr() dsfs() this is the last refuge of the desperate - i.e. when you want to add a feature to another module and there's no other place to make the change. It's all very well to not like 'last resort of the desperate' type hooks, but webchick's list shows us that there's plenty of desperate people out there we gave refuge to.
For schema module, we're already in the situation with field and cache tables (and soon more) never end up in hook_schema(). A contrib module or http://drupal.org/project/schema could add them back dynamically via hook_schema_alter() though, so that they validate. I've personally done this when I add a custom index on client sites - just provides a place other than the update handler to document the hack, but schema module could provide that hook just as well for that case so it's not really an argument either way.
@Dave Reid's use case looks to me like a similar one to the hook implementations webchick pasted - needing to alter the base tables of an entity since there's no other nice way to get the extra information as a base property - this is particularly the case when it comes to bundles, since field_attach_load() gets called before hook_entity_load().
After #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions we should be close to a situation where the schema of content entities could be altered within the entity system, rather than via hook_schema_alter(). If that's the case, then the file entity, as well as most of the implementations that webchick found ought to be covered. But yeah this isn't really viable to remove until that's done and demonstrated to work.
Comment #26
plachI definitely hope that after #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions adding a base or custom field will be just matter of providing its entity field definition. At least that's the goal.
However keep in mind that, unless something changes, core won't support altering entity schema when there is already data stored. We should definitely be able to write the proper migrations to move data from the old schema to the new one, but that is likely to require a contrib module, which would become a dependency to install File entity on an existing site.
Comment #27
mgifford#1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions is now in.
Is adding a base or custom field just matter of providing its entity field definition?
Comment #28
mgiffordComment #29
andypostthere's no more such hook in core