Currently, modules that require settings for content types (i.e. upload.modules enabled/disabled status, comment.modules read-write/read/disabled setting, …) just define variable variables (variable_set('comments_'. $content_type, ...)). That’s rather messy and pollutes the variable namespace. These settings should be stored elsewhere and be associated to the internal content type (which can change).

Follow-ups

Files: 
CommentFileSizeAuthor
#258 content-type-111715-258.patch146.45 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 57,952 pass(es).
[ View ]
#258 interdiff.txt2.34 KBamateescu
#226 comment_menu_alter_items.png24.07 KBandypost
#140 Screen Shot 2013-04-24 at 11.57.11 AM.png87.79 KBheyrocker

Comments

kkaefer’s picture

Status:Active» Needs review
StatusFileSize
new24.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content-types.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is my first patch for this issue. It moves all content type related settings away from the {variable} table into a serialized array in the {node_type} table. The patch also includes an update to migrate the old settings to the new place.

kbahey’s picture

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.

kkaefer’s picture

I am aware of the fact that serialized arrays are not really the best solution, however, I don’t see another way to store these settings. In the {variable} table, these settings would also be stored as serialized PHP variables, so in that regard, there doesn’t change much. I can’t really think of a good application where we would want content type settings to be queryable. Usually, a site has relatively few content types, so sorting in PHP would also be maintainable.

You make a point about querying the database too often with the content type settings moved to the {node_type} table. However, as content types are loaded anyway with each page load that deals with nodes, I don’t see how this could impact performance. Nonetheless, we can perform benchmarks, I agree.

kkaefer’s picture

$ ab -c10 -n500 http://head.drupal/ (without patch)

Time taken for tests: 91.360 seconds
Requests per second: 5.47 [#/sec] (mean)
Time per request: 1827.20 [ms] (mean)
Time per request: 182.72 [ms] (mean, across all concurrent requests)

Connnection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 2
Processing: 697 1808 323.3 1880 3180
Waiting: 696 1808 323.3 1880 3180
Total: 697 1808 323.3 1880 3180

$ ab -c10 -n500 http://head.drupal/ (patch)

Time taken for tests: 92.069 seconds
Requests per second: 5.43 [#/sec] (mean)
Time per request: 1841.38 [ms] (mean)
Time per request: 184.14 [ms] (mean, across all concurrent requests)

Connnection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 925 1829 320.4 1908 3354
Waiting: 924 1829 320.4 1908 3353
Total: 925 1829 320.4 1908 3354

Looks like this patch doesn't really have impact. For the unpatched Drupal, I got #/sec values from 5.23 to 5.55, for the patched version about the same range.

chx’s picture

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?

eaton’s picture

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.

Boris Mann’s picture

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

sun’s picture

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.

<?php
// 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?

<?php
function content_variable_get($type, $var, $default) {
 
$values = variable_get($var, array());
  if (isset(
$values[$type]) {
    return
$values[$type];
  }
  else {
    return
$default;
  }
}
?>
eaton’s picture

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.

sun’s picture

@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’s picture

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

Nick Lewis’s picture

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:

<?php
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.

kkaefer’s picture

StatusFileSize
new28.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content-types_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled the patch for HEAD. Please review and point out bugs.

Nick Lewis’s picture

Status:Needs review» Reviewed & tested by the community
Nick Lewis’s picture

Whoops... ;-)

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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.

dropcube’s picture

Version:6.x-dev» 7.x-dev

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

sun’s picture

Version:7.x-dev» 8.x-dev
mlncn’s picture

Issue tags:+SettingsAPI

Subscribing and tagging.

peterx’s picture

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.

sun’s picture

Title:Move content type settings to a dedicated storage» Convert node/content types into configuration
Component:node system» node.module
Priority:Normal» Major
Status:Needs work» Postponed
Issue tags:-SettingsAPI+Configuration system

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.

sun’s picture

Priority:Major» Normal

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)

sun’s picture

Assigned:Unassigned» sun
Status:Postponed» Active
Issue tags:+Configurables

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. :)

chx’s picture

The issue referenced in #23 has been committed.

xjm’s picture

andypost’s picture

tim.plunkett’s picture

No, this is bundles, not entity types.

swentel’s picture

Assigned:sun» swentel
Status:Active» Needs review
StatusFileSize
new13.84 KB
FAILED: [[SimpleTest]]: [MySQL] 48,052 pass(es), 10 fail(s), and 5 exception(s).
[ View ]

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.

swentel’s picture

StatusFileSize
new22 KB
PASSED: [[SimpleTest]]: [MySQL] 48,118 pass(es).
[ View ]

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

swentel’s picture

Assigned:swentel» sun

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.

znerol’s picture

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

moshe weitzman’s picture

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?

moshe weitzman’s picture

Ideally, we would update the Issue Summary as well.

sun’s picture

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.

sun’s picture

StatusFileSize
new40.36 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

sun’s picture

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

swentel’s picture

StatusFileSize
new11.01 KB
new47.04 KB
FAILED: [[SimpleTest]]: [MySQL] 48,071 pass(es), 123 fail(s), and 29 exception(s).
[ View ]

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

swentel’s picture

Status:Needs review» Needs work
StatusFileSize
new40.91 KB
FAILED: [[SimpleTest]]: [MySQL] 48,092 pass(es), 14 fail(s), and 2 exception(s).
[ View ]

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.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new40.91 KB
FAILED: [[SimpleTest]]: [MySQL] 48,118 pass(es), 14 fail(s), and 2 exception(s).
[ View ]

Meh, one stupid mistake in forum.

sun’s picture

Status:Needs review» Needs work

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.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new34.01 KB
FAILED: [[SimpleTest]]: [MySQL] 48,333 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

yched’s picture

Status:Needs review» Needs work
+++ 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.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new36.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 111715-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

dagmar’s picture

StatusFileSize
new36.19 KB
PASSED: [[SimpleTest]]: [MySQL] 48,886 pass(es).
[ View ]

Rerolled

tim.plunkett’s picture

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

tstoeckler’s picture

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

Gábor Hojtsy’s picture

Issue tags:+D8MI, +language-config

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

Jose Reyero’s picture

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.

YesCT’s picture

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.

tim.plunkett’s picture

StatusFileSize
new665 bytes
new35 KB
FAILED: [[SimpleTest]]: [MySQL] 49,715 pass(es), 16 fail(s), and 16 exception(s).
[ View ]

Updated for #54, and added the manifest update.

tim.plunkett’s picture

Assigned:sun» tim.plunkett
StatusFileSize
new475 bytes
new34.98 KB
PASSED: [[SimpleTest]]: [MySQL] 50,385 pass(es).
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new678 bytes
new34.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-111715-61.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new34.99 KB
PASSED: [[SimpleTest]]: [MySQL] 49,579 pass(es).
[ View ]
YesCT’s picture

sun’s picture

Assigned:tim.plunkett» sun
StatusFileSize
new52.51 KB

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

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

sun’s picture

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.

sun’s picture

StatusFileSize
new85.34 KB
new100.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/lib/Drupal/taxonomy/Tests/Views/TaxonomyTestBase.php.
[ View ]

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.

sun’s picture

StatusFileSize
new7.68 KB
new101.7 KB
FAILED: [[SimpleTest]]: [MySQL] 48,440 pass(es), 154 fail(s), and 60 exception(s).
[ View ]

Fixed a couple of oversights.

sun’s picture

StatusFileSize
new1.38 KB
new101.84 KB
FAILED: [[SimpleTest]]: [MySQL] 48,440 pass(es), 155 fail(s), and 60 exception(s).
[ View ]

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

sun’s picture

StatusFileSize
new10.92 KB
new106.35 KB
FAILED: [[SimpleTest]]: [MySQL] 50,153 pass(es), 41 fail(s), and 32 exception(s).
[ View ]

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');
sun’s picture

StatusFileSize
new4.28 KB
new107.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node.config.76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
  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

sun’s picture

StatusFileSize
new106.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node.config.78.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Merged HEAD (conflicts).

sun’s picture

StatusFileSize
new104.64 KB
FAILED: [[SimpleTest]]: [MySQL] 50,833 pass(es), 23 fail(s), and 262 exception(s).
[ View ]

Jesus. And once more.

sun’s picture

StatusFileSize
new2.95 KB
new105.8 KB
FAILED: [[SimpleTest]]: [MySQL] 50,766 pass(es), 9 fail(s), and 12 exception(s).
[ View ]
  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.
sun’s picture

StatusFileSize
new721 bytes
new105.82 KB
FAILED: [[SimpleTest]]: [MySQL] 50,514 pass(es), 8 fail(s), and 16 exception(s).
[ View ]
  1. Merged HEAD (conflicts).
  2. Updated NodeTypeListController for new Field UI permissions.
yched’s picture

Status:Needs review» Needs work
+++ 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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new105 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

moshe weitzman’s picture

Status:Needs review» Needs work

Anyone available to reroll and review?

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new104.95 KB

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

plach’s picture

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 :)

Berdir’s picture

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?

plach’s picture

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.

webchick’s picture

Priority:Normal» Major

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

heyrocker’s picture

Priority:Major» Critical

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.

Gábor Hojtsy’s picture

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

YesCT’s picture

Berdir’s picture

StatusFileSize
new101.88 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/book/book.module.
[ View ]

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 :)

chx’s picture

Status:Needs review» Needs work

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new6.68 KB
new104.73 KB
FAILED: [[SimpleTest]]: [MySQL] 51,454 pass(es), 686 fail(s), and 88 exception(s).
[ View ]

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

swentel’s picture

StatusFileSize
new6.23 KB
new110.39 KB
FAILED: [[SimpleTest]]: [MySQL] 53,243 pass(es), 19 fail(s), and 11 exception(s).
[ View ]

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.

swentel’s picture

Status:Needs review» Needs work
StatusFileSize
new5.5 KB
new112.96 KB
FAILED: [[SimpleTest]]: [MySQL] 53,219 pass(es), 4 fail(s), and 10 exception(s).
[ View ]

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

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
new111.7 KB
FAILED: [[SimpleTest]]: [MySQL] 52,610 pass(es), 108 fail(s), and 48 exception(s).
[ View ]

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.

swentel’s picture

Status:Needs review» Needs work
StatusFileSize
new112.91 KB
FAILED: [[SimpleTest]]: [MySQL] 51,152 pass(es), 999 fail(s), and 218 exception(s).
[ View ]

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

swentel’s picture

Status:Needs work» Needs review

Just checking whether the reroll was fine though

Berdir’s picture

StatusFileSize
new1.21 KB
new113.48 KB
FAILED: [[SimpleTest]]: [MySQL] 53,641 pass(es), 4 fail(s), and 10 exception(s).
[ View ]

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.

Berdir’s picture

StatusFileSize
new2.28 KB
new113.04 KB
FAILED: [[SimpleTest]]: [MySQL] 53,099 pass(es), 107 fail(s), and 48 exception(s).
[ View ]

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.

Berdir’s picture

Issue summary:View changes

Updated issue summary.

YesCT’s picture

Issue summary:View changes

adding followup section

YesCT’s picture

Issue summary:View changes

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

YesCT’s picture

StatusFileSize
new7.44 KB
new114.13 KB
FAILED: [[SimpleTest]]: [MySQL] 52,989 pass(es), 107 fail(s), and 48 exception(s).
[ View ]
+++ 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.

YesCT’s picture

Status:Needs review» Needs work
StatusFileSize
new3.09 KB
new114.91 KB
FAILED: [[SimpleTest]]: [MySQL] 53,013 pass(es), 107 fail(s), and 48 exception(s).
[ View ]

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)

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.13 KB
new115.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-config-111715-119.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Gábor Hojtsy’s picture

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.

tim.plunkett’s picture

Status:Needs review» Needs work
+++ 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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new115.75 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/comment.module.
[ View ]

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

chx’s picture

Status:Needs review» Needs work
StatusFileSize
new115.69 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new742 bytes
new115.76 KB
FAILED: [[SimpleTest]]: [MySQL] 54,647 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

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

rbayliss’s picture

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

Berdir’s picture

StatusFileSize
new1.32 KB
new115.95 KB
FAILED: [[SimpleTest]]: [MySQL] 54,485 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

swentel’s picture

StatusFileSize
new1.79 KB
new117.13 KB
PASSED: [[SimpleTest]]: [MySQL] 54,566 pass(es).
[ View ]

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 ?

cosmicdreams’s picture

Status:Needs review» Needs work
Issue tags:-D8MI, -language-config
+++ 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.

jibran’s picture

Issue tags:+D8MI, +language-config

tagging

swentel’s picture

Status:Needs work» Needs review

So let's keep it at 'Needs review'

tim.plunkett’s picture

+++ 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

tim.plunkett’s picture

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.

heyrocker’s picture

Status:Needs review» Needs work
StatusFileSize
new87.79 KB

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

Screen Shot 2013-04-24 at 11.57.11 AM.png

Not entirely sure how this is passing tests given that

heyrocker’s picture

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

tim.plunkett’s picture

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

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new10.41 KB
new113.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

swentel’s picture

StatusFileSize
new483 bytes
new113.57 KB
FAILED: [[SimpleTest]]: [MySQL] 54,887 pass(es), 159 fail(s), and 127 exception(s).
[ View ]

Stupid comma

swentel’s picture

StatusFileSize
new545 bytes
new113.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,196 pass(es), 47 fail(s), and 21 exception(s).
[ View ]

stupid entity

tim.plunkett’s picture

StatusFileSize
new19.86 KB
new121.27 KB
PASSED: [[SimpleTest]]: [MySQL] 55,568 pass(es).
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new10.21 KB
new121.85 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new16.37 KB
new127.45 KB
FAILED: [[SimpleTest]]: [MySQL] 55,513 pass(es), 9 fail(s), and 6,550 exception(s).
[ View ]

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.

tim.plunkett’s picture

StatusFileSize
new127.45 KB
FAILED: [[SimpleTest]]: [MySQL] 55,568 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new643 bytes

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

tim.plunkett’s picture

StatusFileSize
new1.1 KB
new127.48 KB
PASSED: [[SimpleTest]]: [MySQL] 55,625 pass(es).
[ View ]

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

swentel’s picture

StatusFileSize
new7.77 KB
new135.38 KB
PASSED: [[SimpleTest]]: [MySQL] 55,601 pass(es).
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new135.32 KB
FAILED: [[SimpleTest]]: [MySQL] 55,907 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

swentel’s picture

Status:Needs review» Needs work

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

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new135.33 KB
PASSED: [[SimpleTest]]: [MySQL] 56,075 pass(es).
[ View ]

Added that

alexpott’s picture

Status:Needs review» Needs work

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()

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new10.05 KB
new137.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,684 pass(es).
[ View ]

Addressed everything but the first point.

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

Also fixed {@inheritdoc}

alexpott’s picture

Assigned:sun» catch
Status:Needs review» Reviewed & tested by the community

This looks great! Over to catch...

catch’s picture

Status:Reviewed & tested by the community» Needs work

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?

Berdir’s picture

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.

catch’s picture

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.

Berdir’s picture

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.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new140 KB
FAILED: [[SimpleTest]]: [MySQL] 55,903 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new8.05 KB

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

andypost’s picture

StatusFileSize
new140.04 KB
PASSED: [[SimpleTest]]: [MySQL] 56,847 pass(es).
[ View ]
new1.35 KB
andypost’s picture

StatusFileSize
new139.94 KB
FAILED: [[SimpleTest]]: [MySQL] 55,898 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
new4.75 KB

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

andypost’s picture

StatusFileSize
new139.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content-type-111715-178.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.18 KB

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

andypost’s picture

StatusFileSize
new139.9 KB
FAILED: [[SimpleTest]]: [MySQL] 57,600 pass(es), 1 fail(s), and 4,640 exception(s).
[ View ]

merge HEAD

andypost’s picture

StatusFileSize
new139.9 KB
PASSED: [[SimpleTest]]: [MySQL] 57,598 pass(es).
[ View ]
new556 bytes

Proper merge

andypost’s picture

StatusFileSize
new139.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content-type-111715-185_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.17 KB
andypost’s picture

andypost’s picture

StatusFileSize
new139.9 KB
PASSED: [[SimpleTest]]: [MySQL] 55,607 pass(es).
[ View ]

re-roll with reverted #185

andypost’s picture

StatusFileSize
new139.23 KB
PASSED: [[SimpleTest]]: [MySQL] 55,439 pass(es).
[ View ]

Another merge

andypost’s picture

Issue summary:View changes

another follow-up mentioned but not opened yet

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

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.

catch’s picture

Assigned:catch» Unassigned
Status:Reviewed & tested by the community» Needs work

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

Gábor Hojtsy’s picture

@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?

andypost’s picture

#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

andypost’s picture

StatusFileSize
new139.38 KB
FAILED: [[SimpleTest]]: [MySQL] 56,645 pass(es), 24 fail(s), and 25 exception(s).
[ View ]

Just a re-roll

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new139.14 KB
FAILED: [[SimpleTest]]: [MySQL] 57,328 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
new1.51 KB

We have no manifests now

catch’s picture

@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().

andypost’s picture

StatusFileSize
new141.55 KB
PASSED: [[SimpleTest]]: [MySQL] 57,421 pass(es).
[ View ]
new17.48 KB

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

andypost’s picture

StatusFileSize
new141.34 KB
PASSED: [[SimpleTest]]: [MySQL] 57,492 pass(es).
[ View ]
new3.52 KB

Remove module property

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -81,35 +82,12 @@ class NodeType extends ConfigEntityBase implements NodeTypeInterface {
   public $description;
-
+public $disabled;

Fixed

andypost’s picture

StatusFileSize
new141.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,704 pass(es), 5 fail(s), and 19 exception(s).
[ View ]
new2.17 KB
andypost’s picture

StatusFileSize
new142.69 KB
PASSED: [[SimpleTest]]: [MySQL] 57,400 pass(es).
[ View ]
new3.6 KB

Fix upgrade path and tests

andypost’s picture

Cant reproduce failures locally

catch’s picture

This is looking much better. Didn't have an in-depth look through this time, but the node hook patch removed a lot of strange things in the config format.

I'm still having trouble figuring out what status: status is though.

Berdir’s picture

"status: status" is the "Published" checkbox, node->status.

andypost’s picture

StatusFileSize
new141.09 KB
FAILED: [[SimpleTest]]: [MySQL] 53,832 pass(es), 889 fail(s), and 251 exception(s).
[ View ]

Merge HEAD, let's see how much broken (patch does not fix entity forms)

andypost’s picture

StatusFileSize
new140.01 KB
FAILED: [[SimpleTest]]: [MySQL] 55,099 pass(es), 854 fail(s), and 230 exception(s).
[ View ]
new8.65 KB

Fix forms and cleanup for node_hook()

Berdir’s picture

+++ b/core/modules/node/node.installundefined
@@ -1086,7 +1086,6 @@ function node_update_8019() {
-    // @todo Drop with node_hook() https://drupal.org/node/2018375
     if ($node_type['base'] !== 'node_content' && Drupal::moduleHandler()->moduleExists($node_type['base'])) {
       $locked[$id] = $node_type['base'];

Looks like you dropped the @todo but not the thing that should actually be dropped :)

andypost’s picture

@Berdir yes, because I need to upgrade node-types that prevents deletion while their modules installed.
Currently only core's forum locks it's node type.

+++ b/core/modules/forum/forum.installundefined
@@ -11,11 +11,10 @@
+  // Do not allow to delete the forum's node type machine name.
+  $locked = Drupal::state()->get('node.type.locked');
+  $locked['forum'] = 'forum';
+  Drupal::state()->set('node.type.locked', $locked);

@@ -120,12 +114,14 @@ function forum_uninstall() {
+  // Allow to delete a forum's node type.
+  $locked = Drupal::state()->get('node.type.locked');
+  unset($locked['forum']);
+  Drupal::state()->set('node.type.locked', $locked);

+++ b/core/modules/node/node.installundefined
@@ -1145,6 +1044,56 @@ function node_update_8018() {
+  // Properties to drop: custom, disabled.
+  $locked = array();
...
+    // Convert 'base' property to state.
+    if ($node_type['base'] !== 'node_content' && Drupal::moduleHandler()->moduleExists($node_type['base'])) {
+      $locked[$id] = $node_type['base'];
+    }
+  }
+  Drupal::state()->set('node.type.locked', $locked);

probably the condition should check custom + module_exists

andypost’s picture

StatusFileSize
new140.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,240 pass(es).
[ View ]
new5.43 KB
andypost’s picture

StatusFileSize
new145.88 KB
FAILED: [[SimpleTest]]: [MySQL] 57,891 pass(es), 85 fail(s), and 35 exception(s).
[ View ]
new0 bytes

Another round of clean-up:
Removed node_type_page_title(), node_type_get_label(), node_type_cache_reset()

node_get_type_label() should use entity_load() for node type.

andypost’s picture

StatusFileSize
new15.11 KB

I think node_type_get_names() should care about language but now it always returns plain config values without care about of label language of the type.

EDIT interdiff added

diffstat
64 files changed, 1409 insertions(+), 1394 deletions(-)

tim.plunkett’s picture

Your interdiff is empty.
I'll be pleasantly surprised if this passes test, I'm the one who made changes to node_type_page_title() and I remember needing it

tim.plunkett’s picture

+++ b/core/modules/comment/comment.moduleundefined
@@ -259,10 +259,10 @@ function comment_menu_alter(&$items) {
+  $items['admin/structure/types/manage/{bundle}/comment/fields']['title'] = 'Comment fields';
+  $items['admin/structure/types/manage/{bundle}/comment/fields']['weight'] = 3;
+  $items['admin/structure/types/manage/{bundle}/comment/display']['title'] = 'Comment display';
+  $items['admin/structure/types/manage/{bundle}/comment/display']['weight'] = 4;

How is this supposed to work? hook_menu_alter cannot parse {bundle} correctly.

andypost’s picture

StatusFileSize
new24.07 KB

@tim.plunkett I've got to debug this to find a way to rename this tabs
comment_menu_alter_items.png

tim.plunkett’s picture

Status:Needs review» Needs work

Yeah, as I said, we needed that. Please revert the changes in #222, and let's just finish this patch.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new145.93 KB
FAILED: [[SimpleTest]]: [MySQL] 57,908 pass(es), 75 fail(s), and 35 exception(s).
[ View ]
new1.76 KB

Reverted back node_type_page_title() let's see last failures

andypost’s picture

StatusFileSize
new145.94 KB
FAILED: [[SimpleTest]]: [MySQL] 58,224 pass(es), 9 fail(s), and 1 exception(s).
[ View ]
new771 bytes

small fix

andypost’s picture

StatusFileSize
new145.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,132 pass(es).
[ View ]
new2.2 KB

Should be green now, reverts comment_modules_enabled()

out of scope: Views handlers uses different sanitization/translation -argument does not use t() for type name

diff --git a/core/modules/node/lib/Drupal/node/Plugin/views/argument/Type.php b/core/modules/node/lib/Drupal/node/Plugin/views/argument/Type.php
index 9d93846..d06040a 100644
--- a/core/modules/node/lib/Drupal/node/Plugin/views/argument/Type.php
+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument/Type.php
@@ -33,11 +33,9 @@ function title() {
     return $this->node_type($this->argument);
   }

-  function node_type($type) {
-    $output = node_type_get_label($type);
-    if (empty($output)) {
-      $output = t('Unknown content type');
-    }
+  function node_type($type_name) {
+    $type = entity_load('node_type', $type_name);
+    $output = $type ? $type->label() : t('Unknown content type');
     return check_plain($output);
   }

diff --git a/core/modules/node/lib/Drupal/node/Plugin/views/field/Type.php b/core/modules/node/lib/Drupal/node/Plugin/views/field/Type.php
index 5a6c25f..6d31ae4 100644
--- a/core/modules/node/lib/Drupal/node/Plugin/views/field/Type.php
+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/Type.php
@@ -45,7 +45,8 @@ public function buildOptionsForm(&$form, &$form_state) {
     */
   function render_name($data, $values) {
     if ($this->options['machine_name'] != 1 && $data !== NULL && $data !== '') {
-      return t($this->sanitizeValue(node_type_get_label($data)));
+      $type = entity_load('node_type', $data);
+      return $type ? t($this->sanitizeValue($type->label())) : '';
     }
     return $this->sanitizeValue($data);
   }

tim.plunkett’s picture

#231: content-type-111715-231.patch queued for re-testing.

tim.plunkett’s picture

+++ b/core/modules/comment/comment.moduleundefined
@@ -277,10 +259,10 @@ function comment_menu_alter(&$items) {
-  $items['admin/structure/types/manage/%/comment/fields']['title'] = 'Comment fields';
-  $items['admin/structure/types/manage/%/comment/fields']['weight'] = 3;
-  $items['admin/structure/types/manage/%/comment/display']['title'] = 'Comment display';
-  $items['admin/structure/types/manage/%/comment/display']['weight'] = 4;
+  $items['admin/structure/types/manage/{bundle}/comment/fields']['title'] = 'Comment fields';
+  $items['admin/structure/types/manage/{bundle}/comment/fields']['weight'] = 3;
+  $items['admin/structure/types/manage/{bundle}/comment/display']['title'] = 'Comment display';
+  $items['admin/structure/types/manage/{bundle}/comment/display']['weight'] = 4;

I still don't get this.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeStorageController.phpundefined
@@ -0,0 +1,27 @@
+class NodeTypeStorageController extends ConfigStorageController {
...
+    \Drupal::cache()->deleteTags(array('node_types' => TRUE));

This can be injected now.

+++ b/core/modules/node/lib/Drupal/node/Tests/Config/NodeImportChangeTest.phpundefined
@@ -0,0 +1,70 @@
+  function setUp() {
...
+  function testImportChange() {
+++ b/core/modules/node/lib/Drupal/node/Tests/Config/NodeImportCreateTest.phpundefined
@@ -0,0 +1,89 @@
+  function setUp() {
...
+  function testImportCreateDefault() {
...
+  function testImportCreate() {

Should be marked public.

alexpott’s picture

+++ b/core/modules/book/book.moduleundefined
@@ -1230,16 +1231,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]);
       // Ensure that the allowed_types array is sorted consistently.
       // @see book_admin_settings_submit()
       sort($allowed_types);

The old key here is numeric... just do $allowed_types[$old_key] = $type->id(); and move on... see book.settings.yml and the sort will blow away your nice keys anyway :)

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -278,13 +278,11 @@ function field_ui_entity_bundle_rename($entity_type, $bundle_old, $bundle_new) {
function field_ui_form_node_type_form_alter(&$form, $form_state) {
   // We want to display the button only on add page.
-  if (empty($form['#node_type']->type)) {
-    $form['actions']['save_continue'] = array(
-      '#type' => 'submit',
-      '#value' => t('Save and manage fields'),
-      '#weight' => 45,
-    );
-    $form['#submit'][] = 'field_ui_form_node_type_form_submit';
+  if ($form_state['controller']->getEntity()->isNew()) {
+    $form['actions']['save_continue'] = $form['actions']['submit'];
+    $form['actions']['save_continue']['#value'] = t('Save and manage fields');
+    $form['actions']['save_continue']['#weight'] = $form['actions']['save_continue']['#weight'] + 5;
+    $form['actions']['save_continue']['#submit'][] = 'field_ui_form_node_type_form_submit';
   }

Everything apart from the change to the if here looks like it might be unrelated change. And there is no discussion in the issue as to why these have to be made.

+++ b/core/modules/forum/forum.moduleundefined
@@ -174,13 +174,13 @@ function forum_menu_local_tasks(&$data, $router_item, $root_path) {
+        if (($type = entity_load('node_type', $type_name)) && node_access('create', $type_name)) {

Let's swap the node_access and entity_load around so you don't have to do the entity_load if you don't have access.

+++ b/core/modules/language/language.moduleundefined
@@ -322,6 +323,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';
+    }
+  }

huh? like why is this change necessary - all we are doing is moving node types to CMI...

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -28,13 +35,20 @@ class NodeFormController extends EntityFormController {
+    $this->settings = $type->getNodeSettings('node');

This doesn't look like it is named correctly... $type->getNodeSettings('node') is pretty weird :) ... I suggest getModuleSettings

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,255 @@
+    if (\Drupal::moduleHandler()->moduleExists('language')) {

If we're converting this to a route we should inject this now...

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,255 @@
+    $label = $form_state['values']['name'];

Missing trim()

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,255 @@
+    $type->type = trim($type->id());

Why trim here... the code did not used to do this... if this is valid then we need to trim in the validate as well.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,255 @@
+    // Do not save settings from vertical tabs.
+    // @todo Fix vertical_tabs.
+    unset($variables['additional_settings__active_tab']);

Can we get a followup issue created to address this? And link to it in the code.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,255 @@
+    // @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]);
+    }
+    // Save or reset persistent variable values.
+    foreach ($variables as $key => $value) {
+      $variable_new = $key . '_' . $type->id();
+      $variable_old = $key . '_' . $type->getOriginalID();
+      if (is_array($value)) {
+        $value = array_keys(array_filter($value));
+      }
+      variable_set($variable_new, $value);
+      if ($variable_new != $variable_old) {
+        variable_del($variable_old);
+      }
+    }

Can we add links to the issues that should address this...

+++ b/core/modules/node/lib/Drupal/node/NodeTypeInterface.phpundefined
@@ -0,0 +1,37 @@
+  /**
+   * 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);

As above... lets rename this getModuleSettings. Also what are Node settings for a given module... is this not just settings for a specific module?

+++ b/core/modules/node/lib/Drupal/node/NodeTypeListController.phpundefined
@@ -0,0 +1,80 @@
+      'data' => check_plain($entity->label()),

Use String::checkPlain()

+++ b/core/modules/node/lib/Drupal/node/NodeTypeListController.phpundefined
@@ -0,0 +1,80 @@
+    $row['description'] = filter_xss_admin($entity->description);

Use Xss::filterAdmin

+++ b/core/modules/node/lib/Drupal/node/NodeTypeListController.phpundefined
@@ -0,0 +1,80 @@
+    if (\Drupal::moduleHandler()->moduleExists('field_ui') && user_access('administer node fields')) {

Can be injected by extending EntityControllerInterface

+++ b/core/modules/node/lib/Drupal/node/NodeTypeListController.phpundefined
@@ -0,0 +1,80 @@
+      '@link' => url('admin/structure/types/add'),

Should inject the url_generator to do this.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeStorageController.phpundefined
@@ -0,0 +1,27 @@
+    \Drupal::cache()->deleteTags(array('node_types' => TRUE));

This can be injected

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,219 @@
+   * @todo Rename to $id.
...
+   * @todo D8: Rename to $label.

Let's get a followup issue created and linked to from here...

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,219 @@
+      else debug('do not create');

Looks like this should go :)

+++ b/core/modules/node/lib/Drupal/node/Tests/Config/NodeImportChangeTest.phpundefined
@@ -0,0 +1,70 @@
+    $staging = $this->container->get('config.storage.staging');
+    $staging->write($node_type_config_name, $node_type);

Need to copy config from active to staging before running an import... use $this->copyConfig($active, $staging); before you write to the staging. Otherwise you'll lose the field config created in setUp().

+++ b/core/modules/node/lib/Drupal/node/Tests/Config/NodeImportCreateTest.phpundefined
@@ -0,0 +1,89 @@
+  function testImportCreate() {
+    $node_type_id = 'import';
+    $node_type_config_name = "node.type.$node_type_id";
+    $node_type_manifest_name = 'manifest.node.type';
+
+    // Simulate config data to import:
+    $src_dir = drupal_get_path('module', 'node_test_config') . '/staging';
+    $this->assertTrue(file_unmanaged_copy("$src_dir/$node_type_config_name.yml", "public://config_staging/$node_type_config_name.yml"));
+
+    // Add the coresponding entries to the current manifest data.
+    $active = $this->container->get('config.storage');
+    $node_type_manifest = $active->read($node_type_manifest_name);
+    $node_type_manifest[$node_type_id] = array('name' => $node_type_config_name);
+
+    // Save the manifests as files in the the staging directory.
+    $staging = $this->container->get('config.storage.staging');
+    $staging->write($node_type_manifest_name, $node_type_manifest);
+
+    // Import the content of the staging directory.
+    $this->configImporter()->import();
+
+    // Check that the content type was created.
+    $node_type = entity_load('node_type', $node_type_id);
+    $this->assertTrue($node_type, 'Import node type from staging was created.');
+  }

There is no manifests anymore... and this test needs the same change to copy config from active to staging before writing to staging as the previous test.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypePersistenceTest.phpundefined
@@ -72,8 +71,8 @@ function testNodeTypeCustomizationPersistence() {
+    $this->assertFalse($forum->isLocked(), 'Forum node type is in the database and is not locked');

Node types are no longer in the database :)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeTest.phpundefined
@@ -130,34 +130,31 @@ function testNodeTypeEditing() {
-    $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.');
-    $this->assertTrue(isset($types['page']) && empty($types['page']->disabled), 'Basic page node type still active.');
+    $this->assertFalse($types['book']->isLocked(), "Book module's node type still active.");
+    $this->assertFalse($types['book']->isLocked(), 'Article node type still active.');
+    $this->assertFalse($types['book']->isLocked(), 'Basic page node type still active.');

huh?

+++ b/core/modules/node/lib/Drupal/node/Tests/PagePreviewTest.phpundefined
@@ -179,6 +179,8 @@ function testPagePreview() {
+   *
+   * @todo This test passes even with revisioning disabled.

I think we should just create an issue and not have an @todo... totally unrelated to this change.

+++ b/core/modules/node/node.installundefined
@@ -1160,6 +1059,56 @@ function node_update_8019() {
+    if ($node_type['base'] !== 'node_content' && Drupal::moduleHandler()->moduleExists($node_type['base'])) {

Can we really assume that the module is going to be enabled during update. ATM the major upgrade instructions say that you should disable all modules before upgrading. I think this is likely to change but... also in tests above we've assumed the node type is still locked if the book module is disabled.

+++ b/core/modules/node/node.moduleundefined
@@ -1591,20 +1281,14 @@ function node_menu() {
   $items['admin/structure/types/manage/%node_type'] = array(
     'title' => 'Edit content type',
     'title callback' => 'node_type_page_title',
     'title arguments' => array(4),
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('node_type_form', 4),
-    'access arguments' => array('administer content types'),
-    'file' => 'content_types.inc',
+    'route_name' => 'node_type_edit',
   );

@@ -1719,16 +1401,17 @@ function node_menu_local_tasks(&$data, $router_item, $root_path) {
-function node_type_page_title($type) {
-  return $type->name;
+function node_type_page_title($type_name) {
+  $type = entity_load('node_type', $type_name);
+  return $type->label();

Can't this be removed and the entity_page_label callback used for admin/structure/types/manage/%node_type instead? Or in fact you are using drupal_set_title in the form anyway... shouldn't we just remove the title/title callback on this menu.

+++ b/core/modules/node/node.moduleundefined
@@ -2463,8 +2167,10 @@ function node_form_system_themes_admin_form_submit($form, &$form_state) {
+  elseif ($node->entityType() == 'node_type') {

I think we should be checking the interface here rather than the string... ie. elseif ($node instanceof NodeTypeInterface) {

alexpott’s picture

Status:Needs review» Needs work
+++ b/core/modules/node/node.installundefined
@@ -1160,6 +1059,56 @@ function node_update_8019() {
+function node_update_8020() {
+  $uuid = new Uuid();
+  // Properties to drop: custom, disabled.
+  $locked = array();
+  // Note: {node_type}.name was the label, .type the machine name.
+  $result = db_query('SELECT * FROM {node_type}')
+    ->fetchAllAssoc('type', PDO::FETCH_ASSOC);
+  foreach ($result as $id => $node_type) {
+    $config = config('node.type.' . $id);
+    // Node type.
+    $config->setData($node_type);
+    $config->set('uuid', $uuid->generate());
+    $config->set('langcode', Language::LANGCODE_NOT_SPECIFIED);
+
+    // Node type settings.
+    $variables = db_query('SELECT name, value FROM {variable} WHERE name IN (:names)', array(
+      ':names' => array(
+        'node_submitted_' . $id,
+        'node_preview_' . $id,
+        'node_options_' . $id,
+      ),
+    ))->fetchAllKeyed();
+    $variables = array_map('unserialize', $variables);
+    // There are not necessarily values for all settings, so pollute defaults.
+    $variables += array(
+      'node_submitted_' . $id => TRUE,
+      'node_preview_' . $id => 1, // DRUPAL_OPTIONAL
+      'node_options_' . $id => array('status', 'promote'),
+    );
+    foreach ($variables as $name => $value) {
+      // Turn e.g. 'node_submitted_ID' into 'submitted'.
+      $name = str_replace(array('node_', '_' . $id), '', $name);
+      $config->set('settings.node.' . $name, $value);
+
+      update_variable_del($name);
+    }
+    $config->save();
+    // Convert 'base' property to state.
+    if ($node_type['base'] !== 'node_content' && Drupal::moduleHandler()->moduleExists($node_type['base'])) {
+      $locked[$id] = $node_type['base'];
+    }
+  }
+  Drupal::state()->set('node.type.locked', $locked);
+}

So hmmm... I think this might need thinking about... (or I may be just over worrying)...

At the moment this will convert all node types... even those not created by node... so this obviously includes core modules like forum but also contrib. Is this the desired behaviour?

alexpott’s picture

Issue summary:View changes

Updated issue summary.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new148.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,019 pass(es).
[ View ]
new18.78 KB

Fixed all except rename getModuleSettings() should be done in follow-up #2026165: Finish node type settings conversion
Upgrade path and should be fixed in follow-up #1872738: Remove Node Type API for in-code node types after Convert node/content types into configuration

New 3 follow-ups are added to summary and @todo comments

andypost’s picture

Language element for menu is landed #1945226: Add language selector on menus
So now we can use this approach here

tim.plunkett’s picture

+++ b/core/modules/node/node.moduleundefined
@@ -1591,20 +1282,14 @@ function node_menu() {
     'title callback' => 'node_type_page_title',

@@ -1719,16 +1402,17 @@ function node_menu_local_tasks(&$data, $router_item, $root_path) {
/**
  * Title callback: Provides the title for a node type edit form.
  *
- * @param $type
- *   The node type object.
+ * @param string $type_name
+ *   The machine name of the node type.
  *
  * @return string
  *   An unsanitized string that is the title of the node type edit form.
  *
  * @see node_menu()
  */
-function node_type_page_title($type) {
-  return $type->name;
+function node_type_page_title($type_name) {
+  $type = entity_load('node_type', $type_name);
+  return $type->label();

As of #2027183: hook_menu() title callback is ignored on routes you can now use entity_page_label() here

alexpott’s picture

Status:Needs review» Needs work
+++ b/core/modules/node/lib/Drupal/node/NodeTypeInterface.phpundefined
@@ -0,0 +1,37 @@
+  /**
+   * 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);

We can rename this to getModuleSettings() and fix the documentation. These are node type settings and Node is unnecessarily capitalised...

alexpott’s picture

+++ b/core/modules/book/book.moduleundefined
@@ -1232,23 +1232,23 @@ function book_type_is_allowed($type) {
+      // @todo Drop sorting https://drupal.org/node/2026159

+++ b/core/modules/book/lib/Drupal/book/BookSettingsForm.phpundefined
@@ -67,6 +67,7 @@ public function submitForm(array &$form, array &$form_state) {
+    // @todo Drop sorting https://drupal.org/node/2026159

This @todo is unnecessary... I'm not 100% certain we need this. See #1933548: Book allowed_types settings repetitive and in under certain conditions can change unexpectedly

alexpott’s picture

Re #241 we should definitely remove the @todo's as I've won't fixed the issue.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new4.24 KB
new147.94 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Rerolled and implemented the feedback from #239, #240 and #241.

amateescu’s picture

StatusFileSize
new1.99 KB
new147.94 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Missed a few renames.

Also, I reviewed the patch and the only odd thing that I found is that we don't include UUID's in the default config files. Isn't the new policy that we have to provide them now?

aspilicious’s picture

Yes we should add default uuids :).

andypost’s picture

StatusFileSize
new147.42 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new7.58 KB

Suppose fixed all. Upgrade path comment needs English better then mine :)

amateescu’s picture

StatusFileSize
new2.73 KB
new148.2 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Let's do that then.

amateescu’s picture

StatusFileSize
new3.93 KB
new147.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]

Huge mess here :( Since @andypost's patch from #246 is very close to mine from #244, I just applied the uuid additions and tweaked a comment.

So, disregard #243, #244 and #247, the attached interdiff is for #246.

About @alexpott's question from #236, I think it's fine (and actually nice of us) to upgrade every content type. Otherwise we would have to provide an update helper function and force every core/contrib/custom module to do it on their own..

Gábor Hojtsy’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeTypeListController.phpundefined
@@ -0,0 +1,125 @@
+    $row['description'] = Xss::filterAdmin($entity->description);
+    $row['operations']['data'] = $this->buildOperations($entity);

I reviewed this for compatibility with operations altering, and based on how buildOpeations() is used and the list method is broken up, it seems all right. Nothing wrong found in that sense.

That is we expect to swiftly provide configuration translation user interface support for this in config_translation as soon as this lands :)

Berdir’s picture

Status:Needs review» Needs work
+++ b/core/modules/book/book.moduleundefined
@@ -1230,25 +1231,24 @@ function book_type_is_allowed($type) {
-    $old_key = array_search($type->old_type, $allowed_types);
+    $old_key = array_search($type->getOriginalID(), $allowed_types);

I think that should be getOriginalId()?

Edit: Ok, this is not added here, so rename is out of scope. Still don't understand why it exists and why 50% of the code uses this and the other part ->original->id()?

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -202,19 +202,19 @@ public function testFieldComponent() {
-    $this->enableModules(array('field_sql_storage', 'field_test', 'node', 'system'));
-    $this->installSchema('node', array('node_type'));
+    $this->enableModules(array('field_sql_storage', 'field_test', 'node', 'system', 'text'));

wondering why text.module is now necessary here?

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -222,7 +222,7 @@ public function testRenameDeleteBundle() {
-    node_type_delete('article_rename');
+    entity_delete_multiple('node_type', array('article_rename'));

Can't we just call $info->delete() here?

+++ b/core/modules/field/field.attach.incundefined
@@ -1189,8 +1189,10 @@ function field_entity_bundle_delete($entity_type, $bundle) {
-  // Clear the cache.
+  // Clear the field cache.
   field_cache_clear();
+  // Clear the entity info cache (contains bundles).
+  entity_info_cache_clear();

This now already happens in entity_invoke_bundle_hook(), should not be necessary here anymore?

Technically, the comment changes also wouldn't be necessary as there's only a single cache that we clear, but doesn't matter (the advantage would be that this hunk would not show up in the patch)

+++ b/core/modules/menu/menu.moduleundefined
@@ -670,7 +670,7 @@ function menu_node_submit(EntityInterface $node, $form, $form_state) {
function menu_form_node_type_form_alter(&$form, $form_state) {
   $menu_options = menu_get_menus();
-  $type = $form['#node_type'];
+  $type = $form_state['controller']->getEntity();

Below this uses $type->type. Should we change this too while we're at it? comment was changed but only because it had to be changed anyway. Don't care...

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -18,6 +18,13 @@
+   * The settings array.

Well, yes, I can see that :)

Something like "Default settings for this content/node type" would be a more useful explanation.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,287 @@
+      'preview' => DRUPAL_OPTIONAL,

Rly? DRUPAL_OPTIONAL? What a great name for a constant :)

Not relevant here of course, I just had to write this down.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,287 @@
+      '#default_value' => $type->label(),

We usually don't use the label() for form default values, as it's still possible to alter this. So I think this should use ->name for now.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,287 @@
+      '#machine_name' => array(
+        'exists' => 'node_type_load',
+        'source' => array('name'),

Do we have a solution yet for making this not depend on a procedural function? :)

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,287 @@
+    $actions['delete']['#access'] = !$this->entity->isLocked() && !$this->entity->isNew();

Probably call $this->entity->access('delete') instead of checking isLocked directly?

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,287 @@
+    // 'theme' conflicts with theme_node_form().
+    // '0' is invalid, since elsewhere we check it using empty().
+    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)));

I think the node_form stuff is gone, so this shouldn't be necessary anymore. And 0 probably simply shouldn't be a valid machine name in general.

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,287 @@
+    $types = node_type_get_names();
+    if ($this->entity->isNew() && in_array($label, $types)) {
+      form_set_error('name', t('The human-readable name %name is already taken.', array('%name' => $label)));

That should be covered with the #machine_name validation above?

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.phpundefined
@@ -0,0 +1,287 @@
+    $type->name = trim($type->label());

Again, would be clearer if it would not use ->label() imho, but I don't care really.

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

We can remove this after #1995048: EntityListController::getOperations() should respect access checks.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,218 @@
+   * @todo D8: Rename to $label. https://drupal.org/node/1872738

Why does this have a D8: prefix?

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,218 @@
+   * @see NodeType::$body_label

I think references like this need the namespace...

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,218 @@
+      'options' => array(
+        'entity_type' => $this->entityType,
+        'entity' => $this,

Hm, interesting. Not specifying this in a callback means we don't have this by default and need to duplicate it everywhere. Might be missing in a few other places?

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/NodeType.phpundefined
@@ -0,0 +1,218 @@
+      // Clear the node type cache, so the new type appears.
+      $storage_controller->resetCache(array($this->id()));

So we're misusing the resetCache() override just for this I guess? Are there other places that call resetCache() or should we simply inline this?

+++ b/core/modules/node/node.api.phpundefined
@@ -964,44 +908,35 @@ function hook_ranking() {
+function hook_node_type_update(\Drupal\node\NodeTypeInterface $type) {
+  if ($type->original->id() != $type->id()) {
+    $setting = variable_get('comment_' . $type->original->id(), COMMENT_NODE_OPEN);
+    variable_del('comment_' . $type->original->id());
+    variable_set('comment_' . $type->id(), $setting);
...
+function hook_node_type_delete(\Drupal\node\NodeTypeInterface $type) {
+  variable_del('comment_' . $type->id());

Should we use the code from language module or something else as example? Otherwise, we will have to remember to update this to config or something else when converting comments...

+++ b/core/modules/node/node.moduleundefined
@@ -372,152 +344,45 @@ function node_type_get_label($name) {
+function node_get_type_label(Entityinterface $node) {

Entit*I*interface.

+++ b/core/modules/node/node.moduleundefined
@@ -615,198 +480,24 @@ function node_field_extra_fields() {
-function node_type_update_nodes($old_type, $type) {
+function node_type_update_nodes($old_id, $new_id) {

Not related to this but this totally needs to be a method on the node storage controller ;)

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

I think @catch already commented on this, is there a reason for this? This does seem to be almost the same as loading the full node type entity?

+++ b/core/modules/rest/lib/Drupal/rest/Tests/DeleteTest.phpundefined
@@ -29,6 +29,12 @@ public static function getInfo() {
+    // Create necessarily entity bundles.
+    $this->drupalCreateContentType(array('type' => 'article'));

Why was this not necessary before? :)

+++ b/core/profiles/standard/standard.installundefined
@@ -24,43 +24,9 @@ function standard_install() {
-  foreach ($types as $type) {
-    $type = node_type_set_defaults($type);
-    node_type_save($type);
-    node_add_body_field($type);

Where is the body field added now?

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new11.96 KB
new146.04 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Nice review! Much better than what I found (missing uuids) :P Just commenting on the points that I didn't touch in this interdiff.

wondering why text.module is now necessary here?
...
Where is the body field added now?

Just for posterity because we discussed this in person, this is needed because the body field is now added in NodeType::postSave()

I think @catch already commented on this, is there a reason for this? This does seem to be almost the same as loading the full node type entity?

Loading the config directly bypasses at least the load hooks, maybe there is a reason for this, but I've no idea tbh :/

Let's see what we broke..

amateescu’s picture

StatusFileSize
new963 bytes
new146.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,975 pass(es), 143 fail(s), and 45 exception(s).
[ View ]

It seems that we still need to validate against a node type name being '0' :-<

Status:Needs review» Needs work

The last submitted patch, content-type-111715-252.patch, failed testing.

tim.plunkett’s picture

Issue tags:+MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION, will handle in #2006624: Implement LocalAction plugins for previously converted routes.

amateescu’s picture

Assigned:Unassigned» amateescu
Issue tags:-MENU_LOCAL_ACTION

Fixing it now :)

amateescu’s picture

Issue tags:+MENU_LOCAL_ACTION

sigh..

amateescu’s picture

Assigned:amateescu» Unassigned
Status:Needs work» Needs review
StatusFileSize
new684 bytes
new146.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,938 pass(es).
[ View ]

There we go.

amateescu’s picture

StatusFileSize
new2.34 KB
new146.45 KB
PASSED: [[SimpleTest]]: [MySQL] 57,952 pass(es).
[ View ]

@Berdir found another small issue, that our default config files are actually in english :)

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Ok, so I think the parts that can be addressed from @catch's, mine and other reviews have been. Things like the strangely named settings are still there, but we got rid of the unnecessary and weird stuff in the .yml files.

I did a relatively thorough review I think, so this should be in a fairly good shape. We also manually tested the UI with the validation stuff for 0 and the installer.

@catch, I think it's your turn again :)

alexpott’s picture

Title:Convert node/content types into configuration» Change notice: Convert node/content types into configuration
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

As this is not my rtbc anymore I think it is acceptable for me to commit...

Committed 46973e7 and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags:-Needs change record

Yippidiyay!

Gábor Hojtsy’s picture

I added #2029405: Write configuration schema for node types in core and #2029407: Add support for node types in config translation as followups.

longwave’s picture

Issue tags:+Needs change record

Fixing tags

amateescu’s picture

andypost’s picture

Status:Active» Needs review

Added https://drupal.org/node/2029519 and updated https://drupal.org/node/1818734

Suppose #2018375: Get rid of node_hook and node_invoke should refer to the same change notice so added about removal at the and of the notice

Gábor Hojtsy’s picture

chx’s picture

Status:Needs review» Fixed

tidied the change notice a little.

plach’s picture

Title:Change notice: Convert node/content types into configuration» Convert node/content types into configuration

Restoring the origianl title.

Status:Fixed» Closed (fixed)

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

andypost’s picture

Maybe better to implement central service to lock entities - #2084567: Implement a EntityLockedInterface and service to allow locking entities

xjm’s picture

Issue tags:-Needs change record

Untagging. Please remove the tag when the change notification task is completed.

xjm’s picture

Issue summary:View changes

added follow-ups

xjm’s picture

Component:node.module» node system
Issue summary:View changes
klonos’s picture

Issue summary:View changes
Parent issue:» #1802750: [Meta] Convert configurable data to ConfigEntity system

...added related issues a children -hid old files ;)