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

Reference: https://www.drupal.org/core/beta-changes
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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
8.09 KB

THis is a try.

dawehner’s picture

Status: Active » Needs review
FileSize
11.6 KB
3.51 KB

Good morning!

Status: Needs review » Needs work

The last submitted patch, 2: 2451395-2.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.82 KB
5.08 KB

Some fixes.

Status: Needs review » Needs work

The last submitted patch, 4: 2451395-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.57 KB
1.72 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, 6: 2451395-6.patch, failed testing.

xjm’s picture

Title: Get rid of drupal_get_schema()/drupal_get_complete_schema() » drupal_get_schema()/drupal_get_complete_schema() no longer work as expected; remove them
Issue summary: View changes
Issue tags: +Needs change record

Agreed 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.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
13.59 KB
1.03 KB

Let's fix the failures.

The CR is as honest as it can get ... you can't access the schema

Crell’s picture

Status: Needs review » Reviewed & tested by the community

It's dead, Jim.

The last submitted patch, 1: 2451395-1.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed db8fa57 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed db8fa57 on 8.0.x
    Issue #2451395 by dawehner: drupal_get_schema()/...
David_Rothstein’s picture

Now that we autogenerate entity schema, we no longer allow people to alter the schema of modules.

I 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"...)

fgm’s picture

Status: Fixed » Needs review
FileSize
2.78 KB

I just went through the code base and can no longer find any place in core where moduleHandler->alter('schema', ...) would be used, meaning that hook_schema_alter() looks like it is no longer invoked and should be removed. The only place where it used to be invoked was in drupal_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 previous drupal_get_schema() ?

dawehner’s picture

Sure why not, making it insane is a bad idea IMHO.

Berdir’s picture

+++ b/core/includes/schema.inc
@@ -157,22 +157,14 @@ function drupal_uninstall_schema($module) {
+ * It is also used by drupal_install_schema() and drupal_uninstall_schema() to
+ * ensure that a module's tables are created exactly as specified.

"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.

Berdir’s picture

Status: Needs review » Needs work
fgm’s picture

Status: Needs work » Needs review
FileSize
11.97 KB

Rerolled. I Removed the "used by" comment since that function is used in multiple places elsewhere anyway, and renamed it as suggested.

alexpott’s picture

Status: Needs review » Needs work

I'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.

  • alexpott committed 6d9f75a on 8.0.x
    Revert "Issue #2451395 by dawehner: drupal_get_schema()/...

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: 0001-Issue-2451395-removed-hook_schema_alter.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
Issue tags: +Performance
Parent issue: » #2470679: [meta] Identify necessary performance optimizations for common profiling scenarios
FileSize
19.63 KB

Quick 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.

alexpott’s picture

Status: Needs review » Needs work
/**
 * Creates all tables defined in a module's hook_schema().
 *
 * Note: This function does not pass the module's schema through
 * hook_schema_alter(). The module's tables will be created exactly as the
 * module defines them.
 *
 * @param string $module
 *   The module for which the tables will be created.
 */
function drupal_install_schema($module) {

and

/**
 * Removes all tables defined in a module's hook_schema().
 *
 * Note: This function does not pass the module's schema through
 * hook_schema_alter(). The module's tables will be created exactly as the
 * module defines them.
 *
 * @param string $module
 *   The module for which the tables will be removed.
 *
 * @return array
 *   An array of arrays with the following key/value pairs:
 *    - success: a boolean indicating whether the query succeeded.
 *    - query: the SQL query(s) executed, passed through
 *      \Drupal\Component\Utility\SafeMarkup::checkPlain().
 */
function drupal_uninstall_schema($module) {

The references to hook_schema_alter can be removed.

catch’s picture

Status: Needs work » Needs review
FileSize
20.27 KB
958 bytes

Removed both references.

The last submitted patch, 24: remove_schema.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Agreed that the alter hook is pointless.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2451395-26.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
20.62 KB

KernelTestBase 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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Alright

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Second time lucky. Committed d5d8b30 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed d5d8b30 on 8.0.x
    Issue #2451395 by dawehner, catch, fgm, David_Rothstein:...

Status: Fixed » Closed (fixed)

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