Get this bug while working on upgrade path for #731724: Convert comment settings into a field to make them work with CMI and non-node entities see #1907960-304: Helper issue for "Comment field" test results

Problem/Motivation

We have ModulesDisabledUpgradePathTest.php that runs updates on disabled modules (comment in that case)
The field-types now works on top of namespace discovery Field.php that does not find any classes because disabled modules excluded from discovery.

Proposed resolution

- Do not run updates on disabled modules
- Try to inject namespace into update container, but that could bring more troubles

#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed

Files: 
CommentFileSizeAuthor
#14 _update_8003_field_create_field-inject_schema-2032369-14.patch4.43 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,646 pass(es).
[ View ]

Comments

andypost’s picture

Title:Upgrade of disabled modules broken» Upgrade of disabled modules broken - no way to use name-space discovery

re-titled

From irc

<swentel> so yeah, I get it know
<swentel> the module is disabled
<swentel> the field type is not found
<swentel> but field_create_field helper function wants to get the schema
<swentel> and boom
<swentel> oh irony
<andypost> suppose this would case more troubles. any discovery based on namespaces affected too
<swentel> andypost: we will probably need a _update_field_sql_stroage_field_storage_create_field()
<swentel> like we actually did have in D7 iirc
<swentel> but I removed that two months ago as we didn't have a use case :D
<andypost> swentel, this affects not only a field types
<swentel> I know
<swentel> we should stop running updates on disabled modules
<swentel> period
Berdir’s picture

I think we should avoid that getSchema() call down there. That's almost like a hook, always was, we just never named it like that, so it still works.

My suggestion would be to make an update path version of that function that expects the field schema as an argument.

It's ugly. But updating will always be ugly, even with enabled modules. We don't load .module files, we might at some point decide that we can no longer load plugins/services, even for enabled modules, as they might do stuff that the partially upgraded site can not yet do.

catch’s picture

Status:Active» Postponed (maintainer needs more info)

My suggestion would be to make an update path version of that function that expects the field schema as an argument.

Yes agreed, that's all we can do.

Marking this 'needs more info'. We shouldn't be loading plugins/services for modules within the upgrade path, if we do, then we should find a way to stop that.

andypost’s picture

Status:Postponed (maintainer needs more info)» Active
Issue tags:+Field API, +Entity Field API

So that means that Field entity should be refactored because we use it in _update_8003_field_create_field()

catch’s picture

Title:Upgrade of disabled modules broken - no way to use name-space discovery» _update_8003_field_create_field() relies on field schemas never changing and plugins being available

https://api.drupal.org/api/drupal/core!modules!field!field.install/funct...

This is the same problem with module schemas - if the field schema changes once head2head updates are supported, then the update will be run with two different schema definitions and subsequent updates can't rely on it. For modules we introduced hook_schema_0() so that when the module is first installed in an update, it's done so with a predictable schema.

It's even worse than that though with fields, because creating a field can happen at any time - it's not just about enabling a module for the first time. So I have no idea how to deal with that.

yched’s picture

This was how the helper function worked in D7 - called hook_field_schema() directly.
Now, of course, the schema() method is in a plugin class, we need to load it (it's a static method though, so no object instanciation needed)

I'm not sure we can do anything with the assumption that the plugin system cannot be used in updates. There's code in there we need to access in updates.

larowlan’s picture

catch’s picture

I'm not sure we can do anything with the assumption that the plugin system cannot be used in updates. There's code in there we need to access in updates.

For every other situation like this in core, we copy the code over, why's that not possible here?

catch’s picture

Removing pointless tag.

yched’s picture

For every other situation like this in core, we copy the code over, why's that not possible here?

In this case this would mean either having the caller of _update_8003_field_create_field() provide the field schema, and either:
- modify the calling stack so that field_sql_storage functions accept an injected schema rather than fetching the schema themselves
- not call field_sql_storage functions and duplicate the whole table creation logic in _update_8003_field_create_field() [note: that's 100+ lines - see _field_sql_storage_schema()]

Both seem a little overkill to support the case of "module providing the field type might be disabled", which we're not supposed to support anyway after "disabled modules" are killed ?

yched’s picture

Hm. So, as the fine folks in #731724: Convert comment settings into a field to make them work with CMI and non-node entities have found out in their workaround for this:

$field = new Field(array
  'name' => 'foo',
  'type' => 'text',
  ...
  'schema' => array(...), // an injected, hardcoded schema array
));

results in the protected $field->schema property being set to the injected schema array.
Since $field->getSchema() (called by field_sql_storage when creating the tables) basically does:

if (!isset($this->schema)) {
  $this->schema = (call $field_type_class::schema($field))
}
return $this->schema;

then the injected schema just wins, without needing to access the field type class.

So, yes, passing a hardcoded field schema in _update_8003_field_create_field() should "just work" currently. It's just that this only happens to work by chance and was never an intended behavior. I guess we'd need to make that explicit somewhere so that it doesn't get broken later on...

andypost’s picture

So what's left here to mark fixed?
We have workaround and probably this in no more critical then?!
So maybe better just document in schema() method the usage for updates?

Berdir’s picture

We should also break it explicitly for enabled modules. As explained by @catch, an update function must not rely on plugins.

yched’s picture

Status:Active» Needs review
StatusFileSize
new4.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,646 pass(es).
[ View ]

Patch
- makes 'schema' required in _update_8003_field_create_field()
- adds it for existing calls
- adds a test to ensure that injection of an explicit schema in a Field config entity works.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Nice!

catch’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:-Field API

Opened #2055605: Disallow use of plugins during update.

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary:View changes

related