Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
Now that we autogenerate entity schema, we no longer allow people to alter the schema of modules.
In other words, we don't use drupal_get_schema() anymore
Proposed resolution
Let's kill it, everything else is a lie.
Beta phase evaluation
Issue category | Task because there isn't actually a functional bug (just the risk of introducing one). |
---|---|
Issue priority | Major because these functions will not work as expected in Drupal 8 and are misleading for core and contrib. |
Prioritized changes | The main goal of this issue is removing mostly-dead code that will not work as expected, and thereby reducing fragility. @alexpott has agreed that this change is worth pursuing during the beta. |
Disruption | The patch will include a BC break. We should break BC rather than having a broken API. |
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#30 | 2451395-30.patch | 20.62 KB | catch |
#26 | interdiff.txt | 958 bytes | catch |
#26 | 2451395-26.patch | 20.27 KB | catch |
#24 | remove_schema.patch | 19.63 KB | catch |
#19 | 0001-Issue-2451395-removed-hook_schema_alter.patch | 11.97 KB | fgm |
Comments
Comment #1
dawehnerTHis is a try.
Comment #2
dawehnerGood morning!
Comment #4
dawehnerSome fixes.
Comment #6
dawehnerThere we go.
Comment #8
xjmAgreed with the major priority; it's almost (but maybe not quite) a bug. At a minimum, these functions (and the documentation for them) are extremely misleading and a source of fragility for that reason. They're also almost dead code at the moment. I spoke with @alexpott and he agrees with making this a prioritized change for that reason.
Comment #9
dawehnerLet's fix the failures.
The CR is as honest as it can get ... you can't access the schema
Comment #10
Crell CreditAttribution: Crell at Palantir.net commentedIt's dead, Jim.
Comment #12
alexpottCommitted db8fa57 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedI am confused - hook_schema_alter() is a documented hook, both before and after this patch. Did it stop working at some point? Or did this issue make it stop working? Or is it still operational somewhere else?
Also not really getting why we'd remove a usefully-named function like drupal_get_schema() and leave behind a confusingly-named one like drupal_get_schema_unprocessed(). What does "unprocessed" mean now? (The function documentation says it returns something "unprocessed and unaltered"...)
Comment #15
fgmI just went through the code base and can no longer find any place in core where
moduleHandler->alter('schema', ...)
would be used, meaning thathook_schema_alter()
looks like it is no longer invoked and should be removed. The only place where it used to be invoked was indrupal_get_complete_schema()
.I also updated the doc comment for
drupal_get_schema_unprocessed()
accordingly.Like David_Rothstein staid, could we not rename the function to avoid this "unprocessed" for better DX, to avoid this impression that some processing (altering) may still happen ? Maybe like
drupal_get_module_schema()
to avoid a name collision with the previousdrupal_get_schema()
?Comment #16
dawehnerSure why not, making it insane is a bad idea IMHO.
Comment #17
Berdir"exactly as specified" doesn't make sense anymore if there is no way to alter it.
It's not like someone is going to not do it exactly as specified (Nahh.. today I don't want to create varchar fields, I'll just make this a date)
+1 to renaming it.
Comment #18
BerdirComment #19
fgmRerolled. I Removed the "used by" comment since that function is used in multiple places elsewhere anyway, and renamed it as suggested.
Comment #20
alexpottI've decided to revert the patch I committed in #12 - that commit was premature. We need to consider the use cases for altering schema - I think that removing hook_schema_alter is the correct thing to do but I think that more actively choosing and documenting that choice is worthwhile - plus the change notice will need more work.
Also the documentation in
Drupal\Core\Database\Schema
needs work since that is still defining the node table.Comment #24
catchQuick re-roll, may have missed something.
Marked #2502373: Don't rebuild the schema on module install as duplicate.
This costs us both execution time on memory on module install/cache clears for absolutely zero benefit.
Comment #25
alexpottand
The references to hook_schema_alter can be removed.
Comment #26
catchRemoved both references.
Comment #28
dawehnerAgreed that the alter hook is pointless.
Comment #30
catchKernelTestBase had a test assertion that just doesn't make sense any more.
DbDumpTest was looking for cache_default which is no longer in the dump because we no longer cache the schema array so the table wasn't created.
Comment #31
dawehnerAlright
Comment #32
alexpottSecond time lucky. Committed d5d8b30 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.