Convert node/content types into configuration

Konstantin

While I agree with you that uncluttering the variable name space Is A Good Thing (TM), I am concerned about the following:

- Performance: the variables in the variable table are read once and cached. Now there is more access to the database to get this info, probably from many tables (e.g for pages having lists of many node types).

- Serialized arrays: this is a personal/philosophical point, but rooted in normalization and data storage. A serialized array is just a blob in the database that cannot be used in a WHERE clause, or ORDER BY or any other operations. The blob has to be read in its entirety and processed in PHP, rather than being done in the back end.

So, I recommend a benchmark before and after to see if this has a negative impact before you proceed further with it.

If you want to test this patch then you want to enable comment & upload and then go to admin/content/types and play with the settings for a content type -- does everything work as it should?

I, too, am concerned about the serialized arrays. I'd prefer a table that stores dedicated settings, keyed by nodetype and module. That would allow content-type changes to migrate settings over easily, and module uninstallation could easily clear out any node-type settings.

+1 on Eaton's comments -- let's refactor the variable table instead.

Think also of install profiles: currently, it is easy and simple to do variable_set() to twiddle lots of settings. I don't think this will necessarily have an effect, but wanted to bring it up.

And, if variable_set() goes away (rather than just routing to a different table) the next patch will have to modify the default.profile to capture the change.

If I read the benchmark correctly, this patch does neither boost nor degrade performance. So I'm inclined to reduce this issue to the cause: String concatenations everywhere.

Simplified to this statement, content type-specific variables could be stored as (serialized) arrays, using one variable per module.

// Instead of
variable_get('comments_'. $content_type, $foo);
variable_set('comments_'. $content_type, $foo);
// do
content_variable_get($node->type, 'comments', $foo);
content_variable_set($node->type, 'comments', $foo);

Instead of introducing a new variable storage, why not implement two new helper functions?

function content_variable_get($type, $var, $default) {
  $values = variable_get($var, array());
  if (isset($values[$type]) {
    return $values[$type];
  }
  else {
    return $default;
  }
}

Sun, I don't think that boosting performance was the goal of the original issue: the benchmark that showed no speed change was meant to indicate that there was no *penalty* for the new approach.

The benefit of treating content type configuration as a separate set of options is that the main variable namespace isn't polluted, and that it's easier to re-sync content type settings when a type is renamed, among other things.

@Eaton: If you look at #8 again, variables are less polluted because each module saves its content type settings in one array for all content types. However, moving all content type variables from one type to another might not be simplified that way.

@Eaton: If you look at #8 again, variables are less polluted because each module saves its content type settings in one array for all content types. However, moving all content type variables from one type to another might not be simplified that way.

Well, that links up with my hope that we simply use a separate storage system for content-type settings... Rather than the serialized arrays. We don't really gain much if all we add is a wrapper function.

I feel a bit like this discussion is getting off track -- its pretty clear to me that serialized arrays is an easy and sensible solution to storing these settings.

I don't see a great deal of value in being able to write "SELECT name FROM {node_vars} WHERE value = '0' ORDER by name ASC". And it seems to me that there are too many gotchas when it comes to storing node settings in neat little columns, for example: is this data serialized, a string, numeric? How are we planning on handling the name space between node settings? Are we going to have to have a new series of tables... e.g. (node_type_variables). There are probably enough small, and subjective questions like this that I think we'd fail to get this patch into drupal 6 unless we settle for serialized arrays for now. I'd really like to see this in 6.

The final result of this patch -- being able to declare default settings for nodes in CODE instead of through an interface is priceless. Haven't had time to verify this, but this appears to be an example of how easy it would be to set default node settings:

function node_node_type($op, $info) {
  switch ($op) {
    case 'defaults':
      return array(
        'node_options' => array(
          'status' => TRUE,
          'promote' => TRUE,
          'sticky' => FALSE,
          'revision' => FALSE,
       ),
     );
     break;
}
}

Far cry from having to fill up a .install file with variable_set($node_type.'_variable_name', TRUE).

So think of it this way:
Does this patch make our code cleaner, and easier to understand? yes.
Does it improve how settings are organized in drupal? yes.
Will it make programming for drupal less time consuming? yes.
Will this make drupal behave better in dev-to-stage-to-production enviroments? F#ck yes.
Will this in the end benefit non programmers? Yes: anything we can do reduce the number of settings someone has to mess with to make their site work is a good thing.

The real question for me is where are the bugs, and oddities in this patch that we need to fix before we include it.

Whoops... ;-)

Did you even test this? :P

The update path needs to be changed to the correct format post-schema API. There don't appear to be docs for this yet, but see the most recent update for an example.

Maybe you guys want to take up this again...

Subscribing and tagging.

An alternative approach: http://drupal.org/node/1369954
Enhance the variable storage functions so a module can store all the variables for the module as one blob. The content type code could use the same enhancement per content type. The change would benefit many modules.

The OO approach would overcome the silly content type settings variations where some are stored as strings in an array while others are booleans as strings . The could all be booleans in an object. There are several other modules with similar inconsistencies that could be fines the same way.

Re-purposing for new configuration system.

Postponing on initial conversion of taxonomy vocabularies in #1552396: Convert vocabularies into configuration, since that revealed a range of shortcomings in the configuration system already that need to be fixed first.

Downgrading priority. If anyone's interested, the architectural discussion for dynamic configuration like this happens in #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)

This is the counter-part to #1779486: Convert Node module variables/settings into configuration now.

I'm on it.

#1564832: Remove _node_extract_type() and node_type_get_name() is a dependency and could use some reviews/love. :)

The issue referenced in #23 has been committed.

Meta issue for these: #1802750: [Meta] Convert configurable data to ConfigEntity system

Seems this duplicate of #1763974: Convert entity type info into plugins

No, this is bundles, not entity types.

Taking over from sun for today and tomorrow (sun, feel free to help along of course). First patch moving content types to config entity.
Uploading patch to see what other failures we'll have. Upgrade path will be fail heavily, but want to see if there are others as well.

This should fix all tests. Removed all references to {node_type} as well.

Re-assigning to sun again. If any reviews come in before tomorrow I can work on it some more, moving back to field api cmi patch again.

Did not do a reality-check on those ideas but probably you should consider:

• Provide default node-types as YAML file in install profiles
• As per #1775842: [meta] Convert all variables to state and/or config systems and #1831600: Convert node_options_T, node_preview_T, node_submitted_ T variables to use configuration system somehow incorporate node_options_ . $type, node_preview_ . $type, node_submitted_ . $type into the config entity, probably also language_content_type_' . $type variables in the config entity.

This looks totally straightforward and good to me.

Do we not need any changes to UI where you create and edit a content type? Also, do we need any typed data changes for the $node->type property? Isn't that a sort of entity reference like uid property?

Ideally, we would update the Issue Summary as well.

Ugh. There's (lots of) code for this already in the CMI sandbox; config-node-111715-sun

I'll have to compare the patch (which looks too simple to me based on its size) with the latest state in there and potentially take over any improvements.

Alright. I should have provided this earlier to indicate "progress", but the current state of affairs is still so anything-but-complete that this didn't cross my mind.

I merged latest HEAD, and also merged all additional/sensible changes of @swentel from the patch in #30.

As mentioned, this is anything but complete. Unlike other config conversions, the pre-existing node type forms are actually a bit more advanced, which means that they need the advanced form handling that is required for entity forms, which in turn means that this conversion will have to diverge from others and should directly do the conversion to EntityFormController as part of the initial config entity conversion.

The much larger offender is the Node Type API itself though. I have no idea how the patch in #30 was able to pass tests, but this conversion is not able to get around major adjustments in the Node Type API functions.

@swentel: Apparently, you did not have write access to the CMI sandbox yet. Now you have. ;)

@sun - I had no idea there was a sandbox for it, thanks for the write access. I've added my own branch but haven't committed the changes yet based on your work.

The reason mine is was so small (and passes) is because I wasn't as ambitious as your changes (eg, not removing hook_node_info for instance). I am/was merely interested to find out if converting node types was going to end up with the same problems that alex and I found out while converting Field API, see #76 / #77, and it is .. ;)

Anyway, Drupal installs now and most tests will pass, but you'll see I had to remove entity_load_multiple and entity_load in node_type_get_types and node_type_load. Not doing that will not even get you passed the installer. Pure inception hell :)

Patch is based on your work, with interdiff attached.

somehow incorporate node_options_ . $type, node_preview_ . $type, node_submitted_ . $type into the config entity

I haven't moved those into the config entity, but as separate config file in'node.settings.$type' since the amount of properties can not be determined since every module can alter the content type form and every item in there is automatically saved. We could store all those on separate property (say 'data') on the config entity, but I'm not in favor of that, but that's my opinion of course :)

Anyway, patch attached goes further on my patch from #30. Renamed 'ContentType' to NodeBundle, and machine name from 'contenttype' to 'node_bundle'. The reason I didn't choose node_type is because that name triggers hooks to be called like comment_node_type_load() in ConfigStorageController->attachLoad() ...

The 'node_options_', 'node_preview_', 'node_submitted_' and 'node_type_language_translation_enabled' are converted. Comment and menu variables are also saved in in the config file (but variable_set() is still used as I didn't want to go through all of those conversions yet) which raises an interesting question whether those settings should actually live in node.settings.$type or in a separate object, thus forcing other modules to add additional submit handlers to save their configuration.

Haven't added an upgrade path test yet, however there's already a test to verify that Blog is reassigned to node module, that's a good start, so I think this one will still be green.

Meh, one stupid mistake in forum.

The reason I didn't choose node_type is because that name triggers hooks to be called like comment_node_type_load() in ConfigStorageController->attachLoad()

hook_node_type_*() being called is a desired effect and required behavior.

The entity type name should definitely be 'node_type' with a corresponding NodeType entity class.

Haha, comment_node_type_load was a menu loader function, oh dear ... Anyway, back to NodeType. I left out the translation variable out for now, only node_preview, node_submitted and node_options are converted. I'll look a bit further moving the node_type into form_state - it looks like I'm simply going the other way around, but I'm afraid that at some point I'll stop as I have no clue to get around a call to entity_load without getting into endless loops.

edit - re: my last remakr, in [#1552396#], the workaround is by using config_get_storage_names_with_prefix() in taxonomy_vocabulary_get_names() - that's afaics the only way to avoid the recursion, so I'm going to keep that approach as well. More changes coming up.

+++ b/core/modules/node/node.moduleundefined
@@ -703,15 +697,15 @@ function _node_types_build($rebuild = FALSE) {
+  // Do not use entity_load_multiple() as we'll end up in a
+  // neverending loop.
+  $content_types = config_get_storage_names_with_prefix('node.type.');
+  foreach ($content_types as $config) {
+    $type_object = (object) config($config)->get();
+    if (!$rebuild && $type_object->disabled) {
+      continue;

More and more patches are adding workarounds for this. Thus, bumped #1822458: Move dynamic parts (view modes, bundles) out of entity_info() to major.
Could we add a @todo with a link to that URL, for easier later tracking ?

7 days to next Drupal core point release.

Should fix the last 3 fails.

Pondering now whether to move node_x to the config entity or not, and thus forcing to write submit handlers for menu, comment and translation - and notifying to everyone in contrib, hey, we don't save your variable anymore.

I'll work on the move to form_state now a bit though.

Rerolled

Wouldn't this also mean that hook_node_info() is gone? Or is that a follow-up?

+++ b/core/modules/node/node.install
@@ -734,6 +630,46 @@ function node_update_8013() {
+/**
+ * Remove the {node_type} table.
+ *
+ * @ingroup config_upgrade
+ */
+function node_update_8015() {
+  db_drop_table('node_type');
+}

I think the new standard is to not actually drop the table in D8 in case any contribs reference it in any way. We should just leave it as is for now and add it to #1860986: Drop left-over tables from 8.x

Re #53: I haven't tried this out, but from the looks of it this patch keeps hook_node_info() in tact for now.

#1552396: Convert vocabularies into configuration just landed, might have patterns to follow, although vocabularies did not have an info hook for example for dynamic vocabularies :) Adding this for D8MI config language tracking too.

Looks good to me. Just wondering:
- Why do we still need node.type.[type] and node.settings.[name] ?
- Why node.type.blog and node.type.page are not configuration files delivered with the modules?

However, I realize it may be the config system that needs some pattern to resolve these kind of issues, so none of this should block this patch.
Actually I think we should commit this one ASAP so we have time to figure out other issues depending on this one, like how the comment configuration for node types will look like / where should it be.

Here is a follow up for #53 (based on #54).

Looks like the suggestion in #54 to leave the table in for now needs feedback. Other than that, this is looking ready.

Updated for #54, and added the manifest update.

Urgh, just made the same mistake in the filter issue. Oh well.

@swentel pointed out I added the manifest for the wrong thing.

Patch context conflicted with #1874300: Remove $entity_type argument from field.module functions that receive a single $entity and #1814916: Convert menus into entities, no actual changes.

In #57 I mentioned a follow up but for got to link to it. Here it is:
#1872738: Remove Node Type API for in-code node types after Convert node/content types into configuration

FWIW, attached is an interdiff between #36 and #65.

I'm going to hack a bit on this now, stay tuned.

Heh. The upgrade path was completely wrecked here. :) {node_type}.name was the human-readable name previously, not the machine name. {node_type}.type was the ID. ;)

That's super-utterly confusing, because you'd expect a config entity that's using a "name" property instead of "id" for its machine name. Changing the 'type' and 'name' properties for node types definitely has to be a follow-up issue. (Not part of this patch, since just that change is going to be giant little monster.)

I also just noticed this gimmick in node_type_save():

  // Saving the content type after saving the variables allows modules to act
  // on those variables via hook_node_type_insert().
  $status = node_type_save($type);

From all the things we've discussed previously, I can't remember whether anyone raised this detail about multidimensional modularity. ;)

That's most probably THE killer argument for putting the settings right into node.type.ID config objects. Otherwise, we're running into quite heavy race conditions in the form of what gets saved first, and which module is able to access which data first, etc.

Still working on this.

Here we go :)

After trying hard to introduce BC layers to no avail, I realized it's taking too long and figured it needs to be done anyway, so I decided to perform a full stack conversion:

1. Fixed upgrade path for node types.
2. Full config entity conversion for node types.
3. Introduced NodeType::getNodeSettings($module): Allows to retrieve the module-specific settings for a specific node type.
4. Fixed upgrade path for UUIDs and node type settings.

The only part I semi-left-out is node_type_form() itself - the NodeTypeFormController simply calls into the existing procedural functions for now. This code can be moved in a follow-up issue.

Manual testing worked fine, some tests also passed. Let's see what the testbot has to say. :)

Code lives in the node-config-111715-sun branch of CMI.

Fixed a couple of oversights.

Fixed bogus node settings access in template_preprocess_node() and node_permissions_get_configured_types().

Fixed various variable name hiccups, cache ID mismatches, excessive cache rebuilds, and #type language_configuration processing.

interdiff (not the patch) contains additional debugging code for a really weird Exception triggered in entity_query(), which is invoked via drupal_cron_run() via field_cron() via field_purge_batch()... When a node type is deleted and Comment module is enabled, then the comment bundle's body field is marked for deletion, and the field purge batch entity_query('comment') bails out, because it cannot find a 'node_type' base table. — There is actually no entity query that would query it though. Instead, the field query seems to get heavily confused by the following part of CommentStorageController::buildQuery(), and actually mistakes 'node_type' as an entity table to join to instead of a field alias:

    // Specify additional fields from the user and node tables.
    $query->innerJoin('node', 'n', 'base.nid = n.nid');
    $query->addField('n', 'type', 'node_type');

1. Fixed fatal error in upgrade path due to superfluous ->execute().
2. Fixed NodeTypePersistenceTest depends on module_list() but does not reset its cache.

Some remaining test failures are caused by this critical bug, which I just filed:
#1889620: config_install_default_config() overwrites existing configuration

Merged HEAD (conflicts).

Jesus. And once more.

1. Fixed stored variables in upgrade path need to be unserialized before default values are added.
2. Fixed REST\DeleteTest.
3. Fixed "Save and manage fields" button does not save the new node type.

1. Merged HEAD (conflicts).
2. Updated NodeTypeListController for new Field UI permissions.

+++ b/core/profiles/standard/config/node.type.page.yml
...
+settings:
+  node:
+    # Not promoted to front page.
+    options:
+      - status
+    # Hide author/submission info.
+    submitted: FALSE

+  /**
+   * Returns the configured Node settings of a given module, if any.
+   *
+   * @param string $module
+   *   The name of the module whose settings to return.
+   *
+   * @return array
+   *   An associative array containing the module's Node settings. Note that
+   *   this can be empty, and default values do not necessarily exist.
+   */
+  public function getNodeSettings($module) {
+    if (isset($this->settings[$module]) && is_array($this->settings[$module])) {
+      return $this->settings[$module];
+    }
+    return array();
+  }

So we're back to talking about node type CMI files being a dumping ground for random data provided by other modules ?

AFAIK, the problems highlighted in #1783346-2: Determine how to identify and deploy related sets of configuration still stand, most notably:

- #1648930: Introduce configuration schema and use for translation : node module needs to provide metadata about its config files. In order to do so, he must collect metadata associated with each "additional data". How will we collect metadata for those 3rd party properties ?

- Updates : when comment module wants to change the name or format of one of its settings, it needs to go and alter the yml files provided by node.module ? That means making assumptions about the filename and structure of those files it doesn't own. I smell nasty cases when node.module wants to change stuff as well.

Re-roll of the last patch. Agree with @yched in #86, we agreed on not doing that in that issue.

Anyone available to reroll and review?

Re-rolled.

The patch currently has an endless loop when translation_entity is enabled, that's why it timed out before and I aborted it. So not testing.

The problem is that translation_entity_entity_info_alter() calls entity_get_bundles(), which obviously requests entity info again. That's just wrong :)

I think that either $entity_info['translation'] or the specific stuff that translation_entity works with should move into a separate hook and not be part of entity_info, as mentioned before :)

Edit: I'm not sure why it doesn't already blow up for terms/vocabularies...?

The problem is that translation_entity_entity_info_alter() calls entity_get_bundles(), which obviously requests entity info again. That's just wrong :)

Originally ET did not call entity_get_bundles() as bundles were available in the entity info and thus they were safe to get in hook_entity_info_alter(). This comment is pretty clear:

    // Check whether translation is enabled at least for one bundle. We cannot
    // use translation_entity_enabled() here since it would cause infinite
    // recursion, as it relies on entity info.

I guess this happened when splitting out bundle info from entity info.

I think that either $entity_info['translation'] or the specific stuff that translation_entity works with should move into a separate hook and not be part of entity_info, as mentioned before :)

If we are setting a pattern after which all dynamic/bundle-dependent data has to be moved outside entity info, this sounds the proper solution. I don't care too much, maybe stopping putting random stuff into the entity info might even be a good thing :)

Yes, it did happen there, but as I said above, no clue why it works for terms/vocabulary, which work just like Node/NodeType in this issue.

Can you maybe get that issue started? I had a quick look but didn't fully understand how that property exactly plays together with what field.module is doing and so on. I'm happy to work on it and e.g. fixing tests but I think I need something to point me into the right direction. E.g, should the whole key be moved to a separate API/Hook or should we just move the translation_entity specific stuff into a separate, specific hook (which would have the advantage of being able to properly document it within translation_entity.api.php)

I don't even really understand why we need to support multiple things there, given that translation_entity is now in core and is *the* way to handle entity/field translations? Why would you not use it?

Can you maybe get that issue started?

Ok, I'll start some work on this ASAP. It seems #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable could be the right place where making this happen. Let's move over there and start discussing.

This feels like at least major; until this is done, we haven't really battle-tested the API.

I actually think its critical. Without this and the Field API conversion, we can't do anything even close to realistic user testing of the system.

This will also make all these config available for translation (once schemas are written for it that is).

#1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable is rtbc (and this issue was waiting on that)

Here's a re-roll, this seems to have a few failures but it at least doesn't result in an endless loop in the translation UI test anymore. Yay :)

There's a failed merge marker in book.module but I am not sure how to resolve it.

I think like this. Also found two others in node.module and a now unecessary yml file for poll.

Should fix a bunch of tests (DUBT tests installing node_type table) and a lot of Field UI tests because node_entity_bundle_info() was broken.

Fixes all tests but EntityTranslationSettingsTest - can't track immediately what is going on there.
NodeType has a 'disabled' property which we can probably swap with 'status'.

This will probably be green. The changes in language.module shouldn't be necessary at all (except node_update hook).

- edit - even that revert doesn't fix it yet - unless I add field_cache_clear(); in the assertSettings() method - but that doesn't feel like the right solution at all.

Re-roll after #1924064: node_type_set_defauts has typo in documentaion got in - oh the irony ... still staring on the EntityTranslationSettingsTest()

Just checking whether the reroll was fine though

That was an interesting one :)

The NodeNG patch changed the instanceof check in node_access() to EntityInterface, which a NodeType is now as well. So we passed that to NodeAccessController (as node is hardcoded there) which then went through all checks, called hook_node_type_access() and as nobody implements that, decided that the user does not have access to create nodes.

This should fix most fails. It's ugly but node_access() will go away anyway.

Don't ask me why that test passes right now.

Note that all the changes in language.module have been added in this issue, the only remaining changes in ther are now related to the node type update hook.

Updated issue summary.

adding followup section

adding a follow-up that was mentioned needs to be opened

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -80,7 +80,7 @@ function field_ui_menu() {
           // Different bundles can appear on the same path (e.g. %node_type and
-          // %comment_node_type). To allow field_ui_instance_load() to extract
+          // %comment_menu_node_type). To allow field_ui_instance_load() to extract
           // the actual bundle object from the translated menu router path
           // arguments, we need to identify the argument position of the bundle
           // name string ('bundle argument') and pass that position to the menu

this needs to be re-wrapped to 80 chars.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -17,6 +17,8 @@
 class NodeFormController extends EntityFormController {
 
+  protected $settings;

this might need a comment block. I made one, but a better description might be needed.
I made the type array, but maybe it should be * @var \Drupal\Component\Utility\Settings
how can I tell?

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,72 @@
+ * @file
+ * Contains Drupal\node\NodeTypeFormController.
...
+   * Overrides Drupal\Core\Entity\EntityFormController::form().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::actions().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::validate().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::save().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::delete().

+++ b/core/modules/node/lib/Drupal/node/NodeTypeStorageController.phpundefined
@@ -0,0 +1,83 @@
+ * Contains Drupal\node\NodeTypeStorageController.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,199 @@
+ * Contains Drupal\node\Plugin\Core\Entity\NodeType.
...
+   * Overrides Drupal\Core\Entity\Entity::id().

this is \Drupal now.
http://drupal.org/node/1354#classes
"If you use a namespace in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash)."

+++ b/core/modules/node/node.api.phpundefined
@@ -1046,44 +1046,35 @@ function hook_ranking() {
+function hook_node_type_delete(\Drupal\node\Plugin\Core\Entity\NodeType $type) {

if this is 'used' we just do the NodeType without the full path.
It's not used so leaving the full path.

+++ b/core/modules/node/node.moduleundefined
@@ -571,10 +484,10 @@ function node_type_save($info) {
+function node_add_body_field(NodeType $type, $label = 'Body') {

check if the @param is typed
it was not. changed it to match this added type.

+++ b/core/modules/node/node.moduleundefined
@@ -655,195 +568,31 @@ function node_field_extra_fields() {
- * @param $old_type
+ * @param $old_id
  *   The current node type of the nodes.
- * @param $type
+ * @param $new_id
  *   The new node type of the nodes.
...
-function node_type_update_nodes($old_type, $type) {
+function node_type_update_nodes($old_id, $new_id) {

can we type these since we are changing them? Are they strings?
I made them be strings.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/DeleteTest.phpundefined
@@ -29,6 +29,12 @@ public static function getInfo() {
+  function setUp() {
+    parent::setUp();

this gets public now.
http://drupal.org/node/325974

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -284,48 +284,31 @@ protected function drupalCreateNode(array $settings = array()) {
-   * @param $settings
+   * @param array $values
...
-  protected function drupalCreateContentType($settings = array()) {
+  protected function drupalCreateContentType($values = array()) {

I think we type hint array in the function definition too.

+++ b/core/profiles/standard/config/node.type.article.ymlundefined
@@ -0,0 +1,8 @@
+settings: {  }

hm. I think this gets saved differently. See point 7 in #1948884-14: Create configuration schemas for block and custom_block modules
Ignoring for now.

+++ b/core/profiles/standard/config/node.type.page.ymlundefined
@@ -0,0 +1,14 @@
+    submitted: FALSE

booleans are saved as '0' '1' I think.

I'm going to check the yml config files that were added and make them match the active saved config that is saved.

this has the configs as they are saved.

I took out the enabled from the forum and book ones.
I think the modified should come out too...

We'll need separate issues to fix the status: status format, similar to #1933548: Book allowed_types settings repetitive and in under certain conditions can change unexpectedly and #1928082: Make usage of book.settings:allowed_types consistent, to make it save as -status

I'm not sure how this relates to yched's concerns (#86)

This should fix the test fails I think. I noticed that in HEAD, that submit callback is *not* called for the language content form and I honestly still don't quit understand why it's suddenly required, was it called manually and we removed that from the node type settings form? That would IMHO be an out of scope change.

Anyway, here's what I did for now:
- Re-add that snippet for #submit
- Added a check to not add it if it's already there
- Added a check to not add it when language_content_settings_form_submit() is already there because that's essentially a duplicate of that callback except with the different form structure.

I don't want to impose config schema inclusion as the initial land criteria for this, so I opened #1958740: Create configuration schemas for content type config entities. However if people feel like including that here, feel free to. We have some people experienced writing schemas now, so we can find help either way.

+++ b/core/modules/node/content_types.incundefined
@@ -333,121 +249,52 @@ function node_type_form_validate($form, &$form_state) {
+  // @todo Remove the entire following code after converting node settings of
+  //   Comment and Menu module.
+  // Remove all node type entity properties.
+  foreach (get_class_vars(get_class($type)) as $key => $value) {
+    unset($variables[$key]);

This definitely looks like it needs to get fixed before commit. Not so sure about all of the other @todos

+++ b/core/modules/node/node.moduleundefined
@@ -256,6 +257,21 @@ function node_entity_display_alter(EntityDisplay $display, $context) {
+function node_type_uri(NodeType $type) {
+  return array(
+    'path' => 'admin/structure/types/manage/' . $type->id(),

I'd rather see this as a method on the NodeType entity, but either way it has to include the entity and entity_type in the 'options' key.

+++ b/core/modules/node/node.moduleundefined
@@ -2014,6 +1757,27 @@ function node_form_block_form_alter(&$form, &$form_state) {
+function node_modules_uninstalled($modules) {
+  // Remove module-specific settings from all node types.
+  $config_names = config_get_storage_names_with_prefix('node.type.');
+  foreach ($config_names as $config_name) {
+    $config = config($config_name);
+    $changed = FALSE;
+    foreach ($modules as $module) {
+      if ($config->get('settings.' . $module)) {
+        $config->clear('settings.' . $module);
+        $changed = TRUE;
+      }
+    }
+    if ($changed) {
+      $config->save();
+    }
+  }

What D7 behavior does this replace? I know that if you alter in arbitrary settings, they get saved, but I didn't know they got cleaned up as well. This just doesn't seem like node.module's job to handle.

Re-roll, conflicted with the changed edit/manage field links weight and the bundle API change.

I think that was just bad timing :) Let's see if this is better.

+++ b/core/modules/node/node.moduleundefined
@@ -1981,7 +1724,7 @@ function theme_node_recent_content($variables) {
+  $output .= theme('maHrk', array('type' => node_mark($node->nid, $node->changed)));

I don't think this was intended.

Right, it wasn't. Also removed the installSchema() call in the failing test.

Fixing last tests.

I think it's time to a) for some in depth reviews, b) start identifying the remaining todo's and c) follow ups no ?

+++ b/core/includes/entity.incundefined
@@ -469,7 +469,13 @@ function entity_form_state_defaults(EntityInterface $entity, $operation = 'defau
+  if ($entity->entityType() . '_form' != entity_form_id($entity, $operation)) {

You could refactor $entity->entityType() . '_form' into a variable so you don't have to concatenate it multiple times.

I'll try to do a more thorough review later.

tagging

So let's keep it at 'Needs review'

+++ b/core/modules/node/content_types.incundefined
@@ -333,121 +249,52 @@ function node_type_form_validate($form, &$form_state) {
+  // @todo Fix vertical_tabs.

What's all this?

+++ b/core/modules/node/content_types.incundefined
@@ -333,121 +249,52 @@ function node_type_form_validate($form, &$form_state) {
+  // @todo Remove the entire following code after converting node settings of
+  //   Comment and Menu module.
+  // Remove all node type entity properties.
+  foreach (get_class_vars(get_class($type)) as $key => $value) {
+    unset($variables[$key]);

Is this a follow-up or a blocker?

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,199 @@
+ *   uri_callback = "node_type_uri"

Booooo. Shoulda been NodeType::uri()

+++ b/core/modules/node/node.moduleundefined
@@ -15,10 +15,11 @@
+use Drupal\file\Plugin\Core\Entity\File;
...
-use Drupal\file\Plugin\Core\Entity\File;

Um, sure :)

+++ b/core/modules/node/node.moduleundefined
@@ -225,12 +226,12 @@ function node_entity_bundle_info() {
-  foreach (node_type_get_names() as $type => $name) {
-    $bundles['node'][$type] = array(
-      'label' => $name,
+  foreach (node_type_get_names() as $id => $label) {
+    $bundles['node'][$id] = array(
+      'label' => $label,
       'admin' => array(
         'path' => 'admin/structure/types/manage/%node_type',
-        'real path' => 'admin/structure/types/manage/' . $type,
+        'real path' => 'admin/structure/types/manage/' . $id,

It's too late now, but these unneeded changes are a bit distracting

+++ b/core/modules/node/node.moduleundefined
@@ -256,6 +257,21 @@ function node_entity_display_alter(EntityDisplay $display, $context) {
+    'path' => 'admin/structure/types/manage/' . $type->id(),

This needs options, with entity and entity_type

I thought I was having some nasty deja vu during that review, and it turns out I gave an almost identical one in #121 but it was never addressed.

I just tried manually testing this and got the following in the installer:

Not entirely sure how this is passing tests given that

So I went to a fresh install without the patch and encountered the same problem, so apparently its an environment problem. Ignore #140.

Nope, what @heyrocker saw was exactly the problem

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,199 @@
+ *   controller_class = "Drupal\node\NodeTypeStorageController",
+ *   form_controller_class = {
+ *     "default" = "Drupal\node\NodeTypeFormController"
+ *   },
+ *   list_controller_class = "Drupal\node\NodeTypeListController",

These have been changed, see #1807042: Reorganizie entity storage/list/form/render controller annotation keys

Lot of changes, so here's a reroll with the interdiff. Re: tim's review of #138

1. This setting would have been committed in the configuration (like it is now in the variable ..) Added additional comment in code.
2. I'd say a follow up.
3. done
4. reverted
5. much simpler now
6. gone with the move of uri() to the class

Let's see what the bot thinks.

Stupid comma

stupid entity

Rerolled. Fixed a couple remaining $form_state['entity'] instances, and moved node_type_form into where it belongs.

Added a NodeTypeInterface, now that we're doing that in the rest of core.

While fixing that bug, I decided we might as well finish the job here and go with routes. Bye bye content_types.inc.
Also cleaned up the baseFormID stuff since that was bothering me.

Ugh. Don't make arbitrary changes to almost RTBC issues.

Okay, restoring node_type_page_title for now. It will have to die later along with the later routing conversions.

And while we're at it, let's add tests, create and update, haven't done delete yet.

Patch fuzz conflicted with #1712250: Convert theme settings to configuration system, no changes, identical diffstat

Failure due to #1292284: Require 'type' key in .info.yml files for node_test_config module

Added that

Patch looks really good... some very minor nits.

+++ b/core/modules/book/book.moduleundefined
@@ -1236,16 +1237,16 @@ function book_type_is_allowed($type) {
-      // Replace the old machine-readable name with the new machine-readable
-      // name.
-      $allowed_types[$old_key] = $type->type;
+      $allowed_types[$type->id()] = $allowed_types[$old_key] ? $type->id() : 0;
+      unset($allowed_types[$old_key]);

This should just be $allowed_types[$old_key] = $type->id(); The keys here are numeric. No need to do the unset()

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,274 @@
+      '#description' => t('Users with the <em>Administer content</em> permission will be able to override these options.'),
+    );
+    if (module_exists('language')) {
+      $form['language'] = array(
+        '#type' => 'details',

\Drupal::moduleHandler() is probably preferred...

+++ b/core/modules/node/lib/Drupal/node/NodeTypeStorageController.phpundefined
@@ -0,0 +1,83 @@
+    foreach ($queried_entities as $id => $entity) {
+      $entity->disabled = !module_exists($entity->module);
+    }

As above... \Drupal::moduleHandler()?

@@ -638,195 +532,31 @@ function node_field_extra_fields() {
+  // @see NodeType::postSave()
   cache()->deleteTags(array('node_types' => TRUE));
...
+  // @todo Possibly needless:
+  entity_get_controller('node_type')->resetCache();

hmmm... I think the functionality in node_type_cache_reset() probably belongs in the resetCache() method on the storage controller and we should not be calling node_type_cache_reset() from inside the controller.

+++ b/core/modules/node/node.moduleundefined
@@ -1163,7 +900,9 @@ function template_preprocess_node(&$variables) {
+  $submitted = config('node.type.' . $node->type)->get('settings.node.submitted');
+  if (!isset($submitted) || $submitted) {

Do we need the !isset() here? don't think so. Also should be using Drupal::config()

Addressed everything but the first point.

NodeType is config entity, so the keys are non-numeric. (right?)

Also fixed {@inheritdoc}

This looks great! Over to catch...

Overall this looks great, but I found several things which look like they've been ported from having to maintain both node_type_save() and hook_node_info() to the config system which could likely be dropped. Also it looks like the docs for hook_node_info() aren't removed by the patch as well as at least one helper function. The structure of the YAML file is also a bit confusing as it stands, but that's largely due to metadata from hook_node_info() collection being ported direct to config as well.

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -56,7 +56,15 @@ public function setOperation($operation) {
   public function getBaseFormID() {
-    return $this->entity->entityType() . '_form';
+    // Assign ENTITYTYPE_form as base form ID to invoke corresponding
+    // hook_form_alter(), #validate, #submit, and #theme callbacks, but only if
+    // it is different from the actual form ID, since callbacks would be invoked
+    // twice otherwise.
+    $base_form_id = $this->entity->entityType() . '_form';
+    if ($base_form_id == $this->getFormID()) {
+      $base_form_id = '';
+    }
+    return $base_form_id;

Not entirely sure about returning an empty string for this. If an entity type doesn't have a base form ID, should it have this method at all? Not a commit blocker at all but don't know if it's worth a follow-up.

+++ b/core/modules/book/config/node.type.book.ymlundefined
@@ -0,0 +1,24 @@
+module: node

Really? Is this still used anywhere?

+++ b/core/modules/book/config/node.type.book.ymlundefined
@@ -0,0 +1,24 @@
+disabled: '0'

The disabled column in the node types table was only added because of hook_node_type() and disabled modules. It shoudn't be necessary for node types as config entity. (Also see below where the code that sets this is removed, I don't see anywhere that sets it now so we could just drop it.

+++ b/core/modules/book/config/node.type.book.ymlundefined
@@ -0,0 +1,24 @@
+custom: '1'

Is custom needed when this is just default config? The book module doesn't require the book node type at all. Again how is this used?

+++ b/core/modules/book/config/node.type.book.ymlundefined
@@ -0,0 +1,24 @@
+modified: '1'

Why would a default node type be modified?

+++ b/core/modules/book/config/node.type.book.ymlundefined
@@ -0,0 +1,24 @@
+      status: status

What's status: status?

+++ b/core/modules/book/config/node.type.book.ymlundefined
@@ -0,0 +1,24 @@
+status: '1'

What's the difference between the status here, and the status in options, and disabled?

+type: forum

this is added, but forum_node_info() isn't removed.

+++ b/core/modules/language/language.moduleundefined
@@ -320,6 +322,18 @@ function language_configuration_element_process($element, &$form_state, &$form)
+  // Do not add the submit callback for the language content settings page,
+  // which is handled separately.
+  if (array_search('language_content_settings_form_submit', $form['#submit']) === FALSE) {
+    // Determine where to attach the language_configuration element submit handler.
+    // @todo Form API: Allow form widgets/sections to declare #submit handlers.
+    if (isset($form['actions']['submit']['#submit']) && array_search('language_configuration_element_submit', $form['actions']['submit']['#submit']) === FALSE) {
+      $form['actions']['submit']['#submit'][] = 'language_configuration_element_submit';
+    }
+    elseif (array_search('language_configuration_element_submit', $form['#submit']) === FALSE) {
+      $form['#submit'][] = 'language_configuration_element_submit';
+    }

Not sure why this hunk is necessary? Looks unrelated superficially.

+++ /dev/nullundefined
@@ -1,493 +0,0 @@
-function node_overview_types() {

mmmm lovely.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,274 @@
+    if (in_array(trim($id), array('0', 'theme'))) {
+      form_set_error('type', t("Invalid machine-readable name. Enter a name other than %invalid.", array('%invalid' => $id)));

Maybe move the invalid types to a helper?

+++ b/core/modules/node/node.moduleundefined
@@ -638,195 +523,28 @@ function node_field_extra_fields() {
-function _node_types_build($rebuild = FALSE) {

Yes!

+++ b/core/modules/node/node.moduleundefined
@@ -638,195 +523,28 @@ function node_field_extra_fields() {
-
-  foreach (module_implements('node_info') as $module) {

Yes!

+++ b/core/modules/node/node.moduleundefined
@@ -638,195 +523,28 @@ function node_field_extra_fields() {
-    $disabled = $type_object->disabled;
-    // Check for node types from disabled modules and mark their types for removal.
-    // Types defined by the node module in the database (rather than by a separate
-    // module using hook_node_info) have a base value of 'node_content'. The isset()
-    // check prevents errors on old (pre-Drupal 7) databases.
-    if (isset($type_object->base) && $type_object->base != 'node_content' && empty($_node_types->types[$type_db])) {
-      $type_object->disabled = TRUE;
-    }
-    if (isset($_node_types->types[$type_db])) {

This is where the disabled property comes from, we don't need that any more.

+++ b/core/modules/node/node.moduleundefined
@@ -879,16 +597,23 @@ function node_rdf_mapping() {
-function node_hook($type, $hook) {
-  $base = node_type_get_base($type);
+function node_hook($id, $hook) {
+  // $id can be NULL for nodes that have been created without a type.
+  if (!isset($id)) {
+    return FALSE;
+  }
+  if (!$type = entity_load('node_type', $id)) {
+    return FALSE;
+  }
+  $base = $type->get('base');
   return module_hook($base, $hook) ? $base . '_' . $hook : FALSE;

If hook_node_info() is gone, so should this be.

+++ b/core/modules/node/node.moduleundefined
@@ -1163,7 +888,9 @@ function template_preprocess_node(&$variables) {
+  // Avoid loading the entire node type config entity here.
+  $submitted = Drupal::config('node.type.' . $node->type)->get('settings.node.submitted') ?: TRUE;

Doesn't this load the config entity? Also why would we avoid it?

The crazy hunk in language.module is necessary because of some changes that we made to the form handling, I don't fully understand how it worked before but it's pretty crazy stuff, see #119 and above, took myself and @swentel quite some time to figure that out.

The node type specific callbacks afaik still exist (hook_form(), hook_submit() and so on), not part of this issue to change that as it's not really related to how the node types are stored.

Well the node type specific callbacks are only called for node types created by hook_node_type() - that hook is no longer invoked and the node types were moved to config consistently, so I don't see how they make sense any more.

Those callbacks don't care how a node type was defined AFAIK, as long as $type->base . '_' . $hook exists as a function it's invoked.

We should absolutely kill that stuff. I see no reason to still support that and it seems that only very very few implementations are left in core (forum_form() being one of them) and I don't see a reason why you can't just use one of the trillion other hooks instead. But it's not related to this patch IMHO.

Removed remains of hoook_node_info() and some cleanups, still needs more work to address #169

Fixed config import after #1890784: Refactor configuration import and sync functions

A bit more clean-up, will try to address disabled/module/status latter

removed modified property and reverted node_type_page_title() back

'module' and 'base' could be merged in one property - they needed only for 'node_hook' and form builder

'disabled' could use default 'status' property of config entity

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,274 @@
+    $form['type'] = array(
+      '#type' => 'machine_name',
+      '#default_value' => $type->id(),
+      '#maxlength' => 32,
+      '#disabled' => $type->locked,
...
+    $form['locked'] = array(
+      '#type' => 'value',
+      '#value' => $type->locked,

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,208 @@
+   * Indicates whether the machine name cannot be changed.
+   *
+   * @var bool
...
+  public $locked = FALSE;

'locked' could be used as 'custom'

+++ b/core/modules/book/config/node.type.book.ymlundefined
@@ -0,0 +1,24 @@
+custom: '1'

+++ b/core/modules/forum/config/node.type.forum.ymlundefined
@@ -0,0 +1,24 @@
+custom: '1'

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,274 @@
+    $form['custom'] = array(
+      '#type' => 'value',
+      '#value' => $type->custom,
...
+    $actions['delete']['#access'] = $type->custom && $type->id();

+++ b/core/modules/node/lib/Drupal/node/NodeTypeListController.phpundefined
@@ -0,0 +1,83 @@
+    if (!$entity->custom) {
+      unset($operations['delete']);

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,208 @@
+  public $custom = TRUE;

+++ b/core/modules/node/tests/modules/node_test_config/config/node.type.default.ymlundefined
@@ -0,0 +1,23 @@
+custom: '1'

'custom' is used to allow delete only... so looks like 'locked'

+++ b/core/modules/node/lib/Drupal/node/NodeTypeStorageController.phpundefined
@@ -0,0 +1,92 @@
+  protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
+    // Ensure that all node types of disabled modules are marked as disabled.
+    foreach ($queried_entities as $id => $entity) {
+      $entity->disabled = !\Drupal::moduleHandler()->moduleExists($entity->module);

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypePersistenceTest.phpundefined
@@ -31,12 +45,12 @@ function testNodeTypeCustomizationPersistence() {
+    $this->assertFalse($forum->disabled, 'Forum node type is not disabled');

@@ -53,17 +67,19 @@ function testNodeTypeCustomizationPersistence() {
+    $this->assertTrue($forum->disabled, 'Forum node type is disabled');
...
+    $this->assertFalse($forum->disabled, 'Forum node type is not disabled');

@@ -72,8 +88,9 @@ function testNodeTypeCustomizationPersistence() {
+    $this->assertTrue($forum->disabled, 'Forum node type is in the database and is disabled');

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeTest.phpundefined
@@ -143,9 +142,9 @@ function testNodeTypeStatus() {
     $this->assertTrue(isset($types['book']) && empty($types['book']->disabled), "Book module's node type still active.");
     $this->assertTrue(isset($types['article']) && empty($types['article']->disabled), 'Article node type still active.');

there's places where disabled property used

Could be refactored in follow-up to use 'status' property of config entity

merge HEAD

Proper merge

Merge after #1892182: #type details: Rename #collapsed to #open

the last patch included reverted #1892182-22: #type details: Rename #collapsed to #open

re-roll with reverted #185

Another merge

another follow-up mentioned but not opened yet

This patch is taking way to long now. Feedback is kicking in to slow. I'm marking this rtbc again, so it get noticed by catch to give a final review.

Most of #169 hasn't been dealt with in the latest patch.

@andypost asked me to reflect on this from @catch in #169:

+++ b/core/modules/language/language.moduleundefined
@@ -320,6 +322,18 @@ function language_configuration_element_process($element, &$form_state, &$form)
+  // Do not add the submit callback for the language content settings page,
+  // which is handled separately.
+  if (array_search('language_content_settings_form_submit', $form['#submit']) === FALSE) {
+    // Determine where to attach the language_configuration element submit handler.
+    // @todo Form API: Allow form widgets/sections to declare #submit handlers.
+    if (isset($form['actions']['submit']['#submit']) && array_search('language_configuration_element_submit', $form['actions']['submit']['#submit']) === FALSE) {
+      $form['actions']['submit']['#submit'][] = 'language_configuration_element_submit';
+    }
+    elseif (array_search('language_configuration_element_submit', $form['#submit']) === FALSE) {
+      $form['#submit'][] = 'language_configuration_element_submit';
+    }

Not sure why this hunk is necessary? Looks unrelated superficially.

The patch removes an explicit addition of this submit callback from the form and looks like it wants to add a generic process routine to add it back without specifics in the concrete forms. Are you saying the submit callback should be specifically added in the form in a one-off way like it is before the patch and the generalisation left for another patch?

#179 explains about properties, node_hook() and friends is out of scope of conversion, same for language element
The only questionable - EntityFormController.php changes and #194

Just a re-roll

We have no manifests now

@andypost I don't think the properties and nook hook are out of scope. Node hook has been for nodes defined by hook_node_info() and the properties like 'disabled' were previously internal to the node type API but now being exposed in default configuration - i.e. the YAML structure is a regression compared to what you used to have to provide for node_type_save().

- Removed 'disabled,locked,custom' in favour of 'node.type.locked' state to allow modules (forum) to lock their types
- Fixed tests
- Changed upgrade path