Files: 
CommentFileSizeAuthor
#170 1552396-interdiff-170.txt2.3 KBandypost
#170 1552396-core-voc-170.patch159.86 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,329 pass(es).
[ View ]
#166 1552396-interdiff-166.txt1.5 KBandypost
#166 1552396-core-voc-166.patch157.77 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,327 pass(es).
[ View ]
#164 1552396-interdiff-164.txt3.2 KBandypost
#164 1552396-core-voc-164.patch156.28 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 49,301 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#156 1552396-interdiff-156.txt1.31 KBandypost
#156 1552396-core-voc-156.patch158.73 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1552396-core-voc-156.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#151 1552396-interdiff-151.txt2.86 KBandypost
#151 1552396-core-voc-151.patch158.72 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,796 pass(es).
[ View ]
#149 1552396-core-voc-149.patch156.41 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,414 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#146 1552396-interdiff-146.txt2.47 KBandypost
#146 1552396-core-voc-146.patch158.8 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,729 pass(es).
[ View ]
#143 1552396-core-voc-143.patch158.04 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,790 pass(es).
[ View ]
#143 1552396-interdiff-143.txt4.93 KBandypost
#140 1552396-core-voc-140-sort-broken.patch154.27 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 48,670 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#140 1552396-core-voc-140.patch154.27 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 48,647 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#136 1552396-core-voc-136-sort-broken.patch154.29 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,549 pass(es).
[ View ]
#136 1552396-core-voc-136.patch154.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1552396-core-voc-136.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#136 1552396-interdiff-136.txt9.61 KBandypost
#133 interdiff-132-133-do-not-test.patch7.92 KBamateescu
#133 1552396-core-voc-133.patch160.23 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 48,314 pass(es).
[ View ]
#132 1552396-interdiff-132.txt1.46 KBandypost
#132 1552396-core-voc-132.patch157.11 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,313 pass(es).
[ View ]
#130 1552396-interdiff-128-130.txt3.65 KBandypost
#130 1552396-core-voc-130.patch157.08 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,313 pass(es).
[ View ]
#128 1552396-core-voc-128.patch157.06 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,195 pass(es).
[ View ]
#124 1552396-core-voc-124.patch157.08 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 47,986 pass(es).
[ View ]
#111 1552396-core-voc-111.patch157.08 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 47,727 pass(es).
[ View ]
#111 1552396-interdiff-111.txt5.05 KBandypost
#104 1552396-core-voc-104.patch153.04 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 47,550 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#100 vocab-1552396-100.patch153.7 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 46,397 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#96 1552396-core-voc-96.patch154.56 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1552396-core-voc-96.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#96 1552396-interdiff-96.txt1.09 KBandypost
#95 1552396-interdiff-93vs95.txt2.29 KBandypost
#95 1552396-core-voc-95.patch154.52 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 46,177 pass(es).
[ View ]
#93 1552396-interdiff-87vs93.txt37.62 KBandypost
#93 1552396-core-voc-93.patch154.35 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 46,174 pass(es).
[ View ]
#91 1552396-interdiff-90.txt23.74 KBandypost
#91 1552396-core-voc-90.patch142.84 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 46,195 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#87 1552396-interdiff-87.txt6.56 KBandypost
#87 1552396-core-voc-87.patch117.56 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 46,094 pass(es), 5 fail(s), and 8 exception(s).
[ View ]
#86 1552396-core-voc-86.patch117.52 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,791 pass(es).
[ View ]
#86 1552396-interdiff-86.txt1.27 KBandypost
#85 1552396-interdiff-85.txt11.12 KBandypost
#85 1552396-core-voc-85.patch117.51 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,792 pass(es).
[ View ]
#84 1552396-core-voc-84.patch108.46 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,110 pass(es).
[ View ]
#84 1552396-interdiff-84.txt3.32 KBandypost
#82 1552396-core-voc-82.patch109.31 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,080 pass(es).
[ View ]
#82 1552396-interdiff-82.txt2.89 KBandypost
#78 1552396-core-voc-78.patch109.35 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,796 pass(es), 213 fail(s), and 245 exception(s).
[ View ]
#78 1552396-interdiff-78.txt2.55 KBandypost
#77 1552396-core-voc-77.patch107.39 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,791 pass(es), 213 fail(s), and 245 exception(s).
[ View ]
#77 1552396-interdiff-77.txt4.39 KBandypost
#72 1552396-core-voc-72-withid.patch110.07 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 41,860 pass(es).
[ View ]
#72 1552396-interdiff.txt8.08 KBandypost
#69 1552396-interdiff-68vs69.txt5.51 KBandypost
#69 1552396-core-voc-69-noid.patch82.08 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 41,676 pass(es).
[ View ]
#69 1552396-core-voc-69-withid.patch110.35 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 41,659 pass(es).
[ View ]
#69 1552396-id-interdiff.txt62.69 KBandypost
#68 1552396-core-voc-68.patch83.94 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 41,497 pass(es).
[ View ]
#68 1552396-interdiff-68.txt1.79 KBandypost
#62 1552396-core-voc-62.patch84.27 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 41,489 pass(es).
[ View ]
#62 1552396-interdiff-62.txt740 bytesandypost
#60 1552396-core-voc-60.patch83.9 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,439 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#60 1552396-interdiff-60.txt7.69 KBandypost
#58 1552396-core-voc-58.patch79.87 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,418 pass(es), 12 fail(s), and 14 exception(s).
[ View ]
#58 1552396-interdiff-58.txt4.23 KBandypost
#56 1552396-core-voc-no-voc-change-56.patch78.8 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,477 pass(es), 19 fail(s), and 12 exception(s).
[ View ]
#56 1552396-interdiff-56.txt632 bytesandypost
#55 1552396-core-voc-55.patch78.73 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,485 pass(es), 16 fail(s), and 12 exception(s).
[ View ]
#55 1552396-interdiff-53vs55.txt2.09 KBandypost
#53 1552396-core-voc-53.patch77.57 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1552396-core-voc-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#53 1552396-core-voc-interdif-50.txt2.95 KBandypost
#50 1552396-core-voc-50.patch78.25 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,248 pass(es), 75 fail(s), and 76 exception(s).
[ View ]
#46 1552396-core-voc-46.patch72.01 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,249 pass(es), 94 fail(s), and 82 exception(s).
[ View ]
#46 1552396-interdiff-44vs46.txt7.84 KBandypost
#44 1552396-core-voc-44.patch66.44 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,211 pass(es), 127 fail(s), and 105 exception(s).
[ View ]
#40 config.taxonomy.40.patch66.05 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.taxonomy.40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#38 config.taxonomy.37.patch64.31 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,436 pass(es), 250 fail(s), and 916 exception(s).
[ View ]
#35 config.taxonomy.35.patch61.45 KBsun
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#19 config.taxonomy.18.patch94.96 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,551 pass(es), 39 fail(s), and 16 exception(s).
[ View ]
#17 config.taxonomy.17.patch91.58 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,579 pass(es), 78 fail(s), and 11 exception(s).
[ View ]
#16 config.taxonomy.16.patch88.9 KBsun
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#8 config.taxonomy.8.patch59.2 KBsun
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PDO Exception detected during run_tests.sh. See review log for details..
[ View ]
#1 drupal8.taxonomy-config.1.patch57.3 KBsun
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new57.3 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Oh my. This his a huge undertaking. Spent the past 6 hours exclusively with this...

Amazing work!

Very clean, I think this simplifies a lot of work on taxonomy terms.

Maybe wrong, but I think the patch includes a couple of changes to image.module that are not related to this issue.

"General error: 3 Error writing file './drupaltestbotmysql/simpletest555283search_dataset.frm' (Errcode: 28)"

Looks like the testbot file system is full.

Is there more context on this task anywhere? We converted vocabs from serial IDs to machine names for D7 so I'm a bit confused as to why they're going back the other way now.

Fails on testbot ... here's the first entries in the error log ...

[Sun Apr 29 10:55:13 2012] [error] [client 10.20.0.126] PHP Fatal error:  Class name must be a valid object or a string in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/entity/entity.module on line 322
[Sun Apr 29 11:04:07 2012] [error] [client 10.20.0.126] PHP Fatal error:  Undefined class constant 'Symfony\\Component\\DependencyInjection\\ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/Symfony/Component/DependencyInjection/ContainerBuilder.php on line 322

#1: drupal8.taxonomy-config.1.patch queued for re-testing.

StatusFileSize
new59.2 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PDO Exception detected during run_tests.sh. See review log for details..
[ View ]

Re-rolled, rebased, merged, and conflict-resolved against HEAD, which was a super-major PITA due to #1533022: Convert taxonomy.module entity classes to PSR-0 changing type hints, phpDoc, and whatnot for vocabularies all over the place. :(

This will most likely still blow up the testbot, but I really wanted to get this re-rolled. Sorry @jthorson :-/

Status:Needs review» Needs work

+++ b/core/modules/forum/forum.module
@@ -219,19 +219,8 @@ function forum_menu_local_tasks_alter(&$data, $router_item, $root_path) {
+  if ($forum_vocabulary_name = variable_get('forum_nav_vocabulary', 0)) {
+    $info['taxonomy_term']['bundles'][$forum_vocabulary_name]['uri callback'] = 'forum_uri';
   }

should be == not =

+++ b/core/modules/image/image.module
@@ -502,32 +502,7 @@ function image_path_flush($path) {
function image_styles() {
-  // @todo Configuration must not be statically cached nor cache-system cached.
-  //   However, there's a drupal_alter() involved here.
-
-//  $styles = &drupal_static(__FUNCTION__);

Do you really wanna remove cache?

+++ b/core/modules/taxonomy/taxonomy.api.php
@@ -11,32 +11,16 @@
-function hook_taxonomy_vocabulary_load(array $vocabularies) {
-  foreach ($vocabularies as $vocabulary) {
-    $vocabulary->synonyms = variable_get('taxonomy_' . $vocabulary->vid . '_synonyms', FALSE);
-  }
-}

Regression? why there's no drupal_alter()?

+++ b/core/modules/taxonomy/taxonomy.module
@@ -428,55 +416,96 @@ function taxonomy_term_access($op, $term) {
+function taxonomy_vocabulary_load_all() {
+  return config_load_all('taxonomy.vocabulary', 'taxonomy_vocabulary_load');
+}

This api calls loader function? also why there's no room to alter?

+++ b/core/modules/taxonomy/taxonomy.module
@@ -428,55 +416,96 @@ function taxonomy_term_access($op, $term) {
+  $config = config('taxonomy.vocabulary.' . $vid);
+  // @todo Ponies! ¶
+  $v = $config->get();
+  if (!isset($v['vid'])) {

comment wtf? trailing whitespace

Status:Needs work» Needs review
Issue tags:-Configuration system

#8: config.taxonomy.8.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, config.taxonomy.8.patch, failed testing.

Status:Needs work» Needs review

Spent the last couple of hours on this patch, converting tons and tons of calling code and tests, only to realize in the end that the actual problem is that $vocabulary is sometimes an array (the config data) and sometimes an object (DrupalConfig $config object).

The original intention was to convert everything to just simply the config data array. This keeps the configuration system mostly out of the module layer implementing the surrounding business logic.

However, this inherently means that the config data cannot contain any meta data that may be needed for business logic in module implementations.

Example:

  $vocabulary['name'] = trim($vocabulary['name']);
  taxonomy_vocabulary_save($vocabulary);
  if (!isset($vocabulary->original)) {
    drupal_set_message(t('Created new vocabulary %name.', array('%name' => $vocabulary['name'])));

Switching everything from arrays to $config objects... will be a monster undertaking (even speaking of this single patch only), since that inherently means this:

  $vocabulary->set('name', trim($vocabulary->get('name')));
  taxonomy_vocabulary_save($vocabulary);
  if (!isset($vocabulary->original)) {
    drupal_set_message(t('Created new vocabulary %name.', array('%name' => $vocabulary->get('name'))));

Now.

How about a crazy idea?

What if we just simply kept the entity classes for these configuration objects?

I.e., Drupal\taxonomy\Vocabulary would remain as-is. Only the entity storage controller goes away. Because we literally swap out the storage, storage engine, and storage controller for vocabularies under the hood only.

I'm very very sleepy right now, but somehow, this makes a lot of sense.

Also, this seems to be fully in line with what we envisioned and discussed for #1448330-19: [META] Discuss internationalization of configuration

I.e., Drupal\taxonomy\Vocabulary would remain as-is. Only the entity storage controller goes away. Because we literally swap out the storage, storage engine, and storage controller for vocabularies under the hood only.

So you mean to implement a config entity storage backend? At least to me, that would be natural approach...

The entity api already offers the storage-related API that a high-level configuration management system should offer for object-like configuration items, so using it makes a lot of sense to me. It's also a DX++ as it means the already known interface (crud functions, hooks, ..) remains valid for configuration objects too.

For taxonomy vocabularies as a kind of semi-entity this approach looks natural but in case of contact forms #1588422-13: Convert contact categories to configuration system this looks like over-engineering but we still need to figure out a way to convert core parts by keeping in mind a internationalization. So I leave #1588422 open/active to care mostly about i18n

@fago: No, I don't want to add a dependency on the entity system. That would be wrong. It would make sense to depend on a generalized property system (then being used by fields, entities, and config), but alas, that doesn't exist yet, so at this point I'm rather thinking of forking the Entity base class and resembling it to act as Config base class.

StatusFileSize
new88.9 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Alright, attached patch retains Drupal\taxonomy\Vocabulary, but refactors it to extend a new ConfigObjectBase+Interface.

I think this makes. a. lot. of. sense. :)

StatusFileSize
new91.58 KB
FAILED: [[SimpleTest]]: [MySQL] 36,579 pass(es), 78 fail(s), and 11 exception(s).
[ View ]

Ah-ha! standard.install is guilty for the testbot failures. That was pretty much my theory since the beginning - the testbot blows up when a test fails right within the setUp() method, before the test is actually executed.

(This also means that all the fatal errors in the review log of #16 point to test cases which are still using the Standard profile and should be converted to the Testing profile ASAP; see #1595028: Convert remaining tests using Standard profile to use Testing profile instead)

Status:Needs review» Needs work

The last submitted patch, config.taxonomy.17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new94.96 KB
FAILED: [[SimpleTest]]: [MySQL] 36,551 pass(es), 39 fail(s), and 16 exception(s).
[ View ]

Attached patch should fix most of the remaining test failures.

Status:Needs review» Needs work

The last submitted patch, config.taxonomy.18.patch, failed testing.

#1054162: Taxonomy bundles not supported by EntityFieldQuery (followup) introduced a entity query alter hook implementation that needs to be removed here.

Side note that the removal of hook_image_styles_alter() might need to be discussed or mentioned for a D7 to D8 update documentation on how you can accomplish the same functionality of that hook in D8.

re: #22: The removal of drupal_alter() for configuration object loads will not be discussed in this issue. It already was in the original #1270608: File-based configuration API. Configuration objects have to be idempotent, which means that config->load()->save()->load() yields the identical result. If you want to change configuration, save it. There will be no other way to do it. (Period.) It looks like we missed to remove this particular drupal_alter() in the initial/original config patch, despite the discussions we had. In any case, if you want to elaborate on this further, please create a separate issue, because the topic is totally irrelevant to this patch.

Fair. I'm simply noting that even though that issue discussed it, this issue is actually removing it and once fixed should be noted in the module upgrade guide how it can be replaced.

How doable is it to split the new API functions and/or the image style clean-up into a separate issue?

Alright, attached patch retains Drupal\taxonomy\Vocabulary, but refactors it to extend a new ConfigObjectBase+Interface.

Interesting. +1 for going the same API for doing the same things, still I'm not so sure about the necessity of the split. A common CRUD-Interface holding the common code/interface together might be an interesting way to explore.

@fago: No, I don't want to add a dependency on the entity system. That would be wrong. It would make sense to depend on a generalized property system (then being used by fields, entities, and config), but alas, that doesn't exist yet, so at this point I'm rather thinking of forking the Entity base class and resembling it to act as Config base class.

Given that, it's likely we produce two separate code paths in all components supporting both, like form generation code.

@dependency: You mean, generally for config objects? I don't see why it should be just "wrong" and would appreciate if someone could explain the reasoning instead of just repeating it again and again :D
I mean, conceptually it totally makes sense, vocabularies clearly are "entities".

Status:Needs work» Needs review

@fago:

  1. A couple of Entity concepts do not exist for (typed) configuration. One good example is ->enforceIsNew(). This method cannot do anything on a config object, because the object's "primary key/ID" is the object and the object's identifier itself. A configuration object only exists once. You can duplicate it, but you cannot multiply it. If that doesn't help, then try to think of UUIDs on entities: An entity's UUID (not its serial ID) maps to the ID/name of a configuration object.
  2. Mimicking most parts of EntityInterface really happens for DX reasons only. However, the actual object rather shares methodologies with the DrupalConfig manager/controller, which (obviously) needs to be untangled, but nevertheless, the semantics of ->load() and ->save() are following DrupalConfig, not Entity. (which is also why I wrote a big phat @todo on the question of whether Vocabulary shouldn't actually be a ConfigObject/DrupalConfig object, instead of wrapping it.)
  3. There's also a @todo on the topic that the configuration system actually does not foresee any module hooks and alter hooks. Two reasons:

    First, configuration must be idempotent; i.e., config->load()->save()->load() must yield the same result.

    Second, configuration is "namespaced". Which means that you generally have no business in my configuration object. You create your own, in the same way you used your own database tables or variables before.

    These are the main reasons for why I did not move the module system related logic into the Vocabulary class, but instead, kept that in the procedural taxonomy_vocabulary_*() functions.

    Admittedly, this is debatable to some extent. Because it inherently means that vocabulary configuration can be changed, sometimes notifying other modules, and sometimes not - depending on whether you loaded/saved the config object via Vocabulary or just plain config().

    The general baseline of thought, however, is that all configuration should work regardless of bootstrap level and state. In fact, the configuration system aims to work as the very first bootstrap phase, DRUPAL_BOOTSTRAP_CONFIGURATION, in which pretty much nothing aside from settings.php exists.

    In turn, that's why I'm very hesitant to introduce a dependency on the entire entity system (and in turn, all of its own dependencies).

    But yes, that's totally the essence we need to discuss here. :)

  4. Lastly, bear in mind that there is "typed" and type-less configuration (Vocabulary vs. random module settings forms).

Status:Postponed» Needs review
Issue tags:-VDC

Thanks!! Here are my thoughts:

A couple of Entity concepts do not exist for (typed) configuration. One good example is ->enforceIsNew().

True. I think it doesn't apply to all entities either, but i don't see a big problem in that. If you use it, the system should just go and try to create something new then - whatever that means for your storage backend.

First, configuration must be idempotent; i.e., config->load()->save()->load() must yield the same result.

Sure, entities should be too, i.e. you should get back what you store. So whether we need the load hook, is just a question of whether we allow modularly built objects - where we are at:

Second, configuration is "namespaced". Which means that you generally have no business in my configuration object. You create your own, in the same way you used your own database tables or variables before.

Hm, so you say it's not valid for any module to extend an existing configuration object? Right now, that's quite common: Think of all the serialized settings and options array, which contrib extensions use to easily add their settings. E.g. as it is for fields. How would that work then?

These are the main reasons for why I did not move the module system related logic into the Vocabulary class, but instead, kept that in the procedural taxonomy_vocabulary_*() functions.

Admittedly, this is debatable to some extent. Because it inherently means that vocabulary configuration can be changed, sometimes notifying other modules, and sometimes not - depending on whether you loaded/saved the config object via Vocabulary or just plain config().

Imo, this would result in a rather bad DX. Moreover, it might result in lots of configuration changes being "lost" in the sense that modules miss them because the update happened via the "wrong channel". I do think that every configuration changes *has to* result in the associated hooks to be invoked, or the whole configuration API is becoming unreliable (and so a mess..).

That said, yes - configuration changes and reloading from disk need a high bootstrap level and I don't see a problem with possibly invoking entity hooks here.

The general baseline of thought, however, is that all configuration should work regardless of bootstrap level and state. In fact, the configuration system aims to work as the very first bootstrap phase, DRUPAL_BOOTSTRAP_CONFIGURATION, in which pretty much nothing aside from settings.php exists.

Of course this has to be valid for the configuration system per se. But that doesn't mean we can introduce a system for handling configuration objects that runs on a higher level + stores to the config system.

In turn, that's why I'm very hesitant to introduce a dependency on the entire entity system (and in turn, all of its own dependencies).

There would be no dependency of the configuration system on the entity system. Modules, that would use the entity system to handle their configuration objects would have to depend on it, yes - but I don't see a problem with that.

Lastly, bear in mind that there is "typed" and type-less configuration (Vocabulary vs. random module settings forms).

Yes, I agree that the config of module setting forms doesn't match the entity concept, thus I don't argue for them to become entities.

But yes, that's totally the essence we need to discuss here. :)

Yep. I think, first it's important to lay out the needed and wanted "features" (hooks to be called, ..) for the configuration objects, then we can evaluate and decide whether the entity system would be a good fit or not.

Issue tags:+VDC

Tagging for now. I do think we should spin off ConfigObjectBase into its own issue, though, because it's way out of scope here and merits consolidating with the work we did at tcdrupal for VDC. We can postpone this on that as needed.

At this point, splitting ConfigObjectBase into a separate issue does not make sense to me, since that would inherently ignore the actual use-case.

That said, there's a major clash in terminology/arch-design. @beejeebus is "also" working on a ConfigObject in parallel, but that is going to be used internal within the config system only (i.e., contrary to this here, which is actively instantiated from the module implementation layer and used within, but not actually passed back to the config system).

Note that #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation partially duplicates this discussion to some extent now. Might be worth to discuss the general architectural problem space over there first - but perhaps not. Let's try. I'll try to make sure that we don't duplicate, or otherwise mark that issue as duplicate.

At this point, splitting ConfigObjectBase into a separate issue does not make sense to me, since that would inherently ignore the actual use-case.

This doesn't make sense to me. There's way more than one actual use-case?

My point is that the underlying "big idea" in this patch has lots of implications for CMI, the entity system, VDC, etc. and so there should really have a dedicated issue rather than several people working on similar solutions in parallel.

I completely agree with xjm, at the very least we should change the title of this issue to match the discussion that is going on.

Ultimately I don't really follow what happened in #12 that led to the idea that moving to typed config objects and a more entity-like api was a necessary change. I would really love some elaboration on that. In other issues I saw it discussed that this would allow the ability to centralize business logic in class-specific ->save() operations and the like, however in #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation it appears there is some consensus forming that this would be unnecessary anyways. However if we had an issue that was just 'Implement typed config classes' we could discuss all these merits and demerits in one place rather than around 15 implementation issues you know?

Issue tags:+VDC
StatusFileSize
new61.45 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Here we go again.

Attached patch converts vocabularies from Entity to ConfigurableBase. Furthermore, it replaces $vid with the vocabulary machine name and drops $machine_name (like all patches before).

Code lives in the config-taxonomy-entity-1552396-sun branch of CMI.

Had to revert the EntityInterface/StorableInterface change for ConfigurableInterface, because Taxonomy vocabularies are using the EntityFormController already. We shouldn't have done that in the first place. If we really want/need a separation, then we need to introduce a truly new ContentInterface that extends EntityInterface. But as already mentioned in Munich, there's not going to be a need for that separation anyway.

Status:Needs review» Needs work

Rather than do that, switch EntityFormController to use StorableInterface.

If you want a follow-up to change this back, please open another issue. We've already converted Views to StorableInterface, and burying this change in an unrelated issue is not fair.

Status:Needs work» Needs review
StatusFileSize
new64.31 KB
FAILED: [[SimpleTest]]: [MySQL] 40,436 pass(es), 250 fail(s), and 916 exception(s).
[ View ]

Created #1761048: Revert StorableInterface/EntityInterface separation

Fixed stale machine_name in Forum module.
Fixed missing dependency on Config module.

Status:Needs review» Needs work

The last submitted patch, config.taxonomy.37.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new66.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.taxonomy.40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed Standard install profile.

Status:Needs review» Needs work
Issue tags:-Configuration system, -VDC

The last submitted patch, config.taxonomy.40.patch, failed testing.

Status:Needs work» Needs review

#40: config.taxonomy.40.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +VDC

The last submitted patch, config.taxonomy.40.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new66.44 KB
FAILED: [[SimpleTest]]: [MySQL] 41,211 pass(es), 127 fail(s), and 105 exception(s).
[ View ]

Re-roll, with a few fixes (mostly about class names)

EDIT: filed #1789722: Fix ConfigStorageController::save

The last submitted patch, 1552396-core-voc-44.patch, failed testing.

StatusFileSize
new7.84 KB
new72.01 KB
FAILED: [[SimpleTest]]: [MySQL] 41,249 pass(es), 94 fail(s), and 82 exception(s).
[ View ]

Fixed
- some tests
- Vocabulary reorder (still Warning at taxonomy.module 1065)
uasort() [function.uasort]: Array was modified by the user comparison function - taxonomy_vocabulary_load_multiple()
- ConfigStorageController::save

A small nit for if this gets re-rolled...

+++ b/core/modules/config/lib/Drupal/config/ConfigStorageController.phpundefined
@@ -283,18 +283,18 @@ class ConfigStorageController implements EntityStorageControllerInterface {
+    if ($config->isNew()) {
       $return = SAVED_NEW;
       $config->save();
-      $this->postSave($entity, TRUE);
-      $this->invokeHook('update', $entity);
+      $entity->enforceIsNew(FALSE);
+      $this->postSave($entity, FALSE);
+      $this->invokeHook('insert', $entity);

On the first couple of read throughs of the patch, this section was confusing. Perhaps enforceIsNew() is misnamed. However, it was confusing why in the isNew() section this was being called with a FALSE parameter. A comment above that line would be really helpful.

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-46.patch, failed testing.

That entire hunk has no business being in this patch, it changes the behavior for all ConfigEntity implementations.
If there is a bug with it, there should be a separate issue with a test.

Status:Needs work» Needs review
StatusFileSize
new78.25 KB
FAILED: [[SimpleTest]]: [MySQL] 41,248 pass(es), 75 fail(s), and 76 exception(s).
[ View ]

I already filed #1789722: Fix ConfigStorageController::save
But this patch require this hunk while this broken

Changes:
- Fixed some tests
- There's no implementation of EntityStorageControllerInterface::loadByProperties() so a few lines are commented in test
- Sort of vocabularies throws Warning
- testTaxonomyVocabularyLoadReturnFalse() is commented out - make no sense

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-50.patch, failed testing.

Currently we have no hooks for moment of config application so this issue should add a reaction for fields first to do something when some vocabulary object beed removed by applying new config

Status:Needs work» Needs review
StatusFileSize
new2.95 KB
new77.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1552396-core-voc-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The VocabularyStorageController:postSave() called original:delete() before hook_update()

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.09 KB
new78.73 KB
FAILED: [[SimpleTest]]: [MySQL] 41,485 pass(es), 16 fail(s), and 12 exception(s).
[ View ]

Fixed forum tests by disallowing changing machine-name for forum, I think we need to disable changing machine name because then we need massive update for taxonomy_term_data

StatusFileSize
new632 bytes
new78.8 KB
FAILED: [[SimpleTest]]: [MySQL] 41,477 pass(es), 19 fail(s), and 12 exception(s).
[ View ]

Let's try disable vid change in form controller

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-no-voc-change-56.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.23 KB
new79.87 KB
FAILED: [[SimpleTest]]: [MySQL] 41,418 pass(es), 12 fail(s), and 14 exception(s).
[ View ]

Most of tests should be green.
we need to call resetCache() to clean-up static taxonomy_vocabulary_get_names
Commented out a test because changing of machine name is not allowed

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-58.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+D8MI
StatusFileSize
new7.69 KB
new83.9 KB
FAILED: [[SimpleTest]]: [MySQL] 41,439 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Another round, most of tests are fixed

Config module is enabled in hook_update() to fix upgrade path tests, we still depends on #1785974: Move ConfigEntity into a Core component

Sorting still throws error: uasort(): Array was modified by the user comparison function so just prefixed with @ - as suggested in https://bugs.php.net/bug.php?id=50688

Also needs D8MI review on default language for vocabularies

<?php
  $result
= db_query('SELECT * FROM {taxonomy_vocabulary}');
  foreach (
$result as $vocabulary) {
   
$config = config('taxonomy.vocabulary.' . $vocabulary->machine_name)
      ->
set('vid', $vocabulary->machine_name)
      ->
set('name', $vocabulary->name)
      ->
set('uuid', !empty($vocabulary->uuid) ? $vocabulary->uuid : $uuid->generate())
      ->
set('description', $vocabulary->description)
      ->
set('hierarchy', $vocabulary->hierarchy)
      ->
set('weight', $vocabulary->weight)
     
// @todo Re-visit later when property patch lands.
     
->set('langcode', language_default()->langcode)
      ->
save();
  }
?>

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-60.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new740 bytes
new84.27 KB
PASSED: [[SimpleTest]]: [MySQL] 41,489 pass(es).
[ View ]

Suppose tests should be green

Overall this patch is AWESOME! Even with all of the update code we're removing 100 lines.

+++ b/core/modules/config/lib/Drupal/config/ConfigEntityBase.phpundefined
@@ -94,7 +94,9 @@ abstract class ConfigEntityBase extends Entity implements ConfigEntityInterface
       $a_label = $a->label();
+      $a_label = isset($a_label) ? $a_label : '';

This could be rewritten as $a_label = $a->label() ?: '';, like we do for the new state() code.

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/DependencyTest.phpundefined
@@ -138,7 +138,7 @@ class DependencyTest extends ModuleTestBase {
-    $expected_order = array('comment', 'options', 'taxonomy', 'php', 'poll', 'forum');
+    $expected_order = array('comment', 'config', 'options', 'taxonomy', 'php', 'poll', 'forum');

Beware, if #1785974: Move ConfigEntity into a Core component lands this will change back

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Term.phpundefined
@@ -102,6 +92,6 @@ class Term extends Entity implements ContentEntityInterface {
-    return $this->vocabulary_machine_name;
+    return $this->vid;

So is this really what we want to do? Or would it make sense to globally renamed vid to machine_name or something that doesn't imply a serial ID?

It seems that half of this patch is renaming vid to machine_name, how much bigger/smaller would it be if we went the other way?

I don't get "Re-visit later when property patch lands", since the config entities should have a language property as well already? The existing vocabularies are saved with the site default language as well right? I see at least that for forums in the patch.

@Gabor, yes, because we have language upgrade tests. But I can't find a code which adds a langcode field for vocabulary... and I scare that my hack (assign a site default language) is wrong...

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.phpundefined
@@ -89,7 +89,7 @@ class LanguageUpgradePathTest extends UpgradePathTestBase {
     // A langcode property was added to vocabularies and terms. Check that
     // existing vocabularies and terms got assigned the site default language.
-    $vocabulary = db_query('SELECT * FROM {taxonomy_vocabulary} WHERE vid = :vid', array(':vid' => 1))->fetchObject();
+    $vocabulary = taxonomy_vocabulary_load('tags');
     $this->assertEqual($vocabulary->langcode, 'ca');
     $term = db_query('SELECT * FROM {taxonomy_term_data} WHERE tid = :tid', array(':tid' => 1))->fetchObject();
     $this->assertEqual($term->langcode, 'ca');

Here is a upgrade tests that we have

+++ b/core/modules/taxonomy/taxonomy.installundefined
+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -278,6 +278,8 @@ function taxonomy_update_8003() {
@@ -278,6 +278,8 @@ function taxonomy_update_8003() {
       ->set('description', $vocabulary->description)
       ->set('hierarchy', $vocabulary->hierarchy)
       ->set('weight', $vocabulary->weight)
+      // @todo Re-visit later when property patch lands.
+      ->set('langcode', language_default()->langcode)

But here the vocabulary table does not have langcode field yet... which is strange...

EDIT

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -264,21 +199,13 @@ function taxonomy_field_schema($field) {
function taxonomy_update_8001() {
   $descriptions = array(
     'taxonomy_term_data' => 'The {language}.langcode of this term.',
-    'taxonomy_vocabulary' => 'The {language}.langcode of this vocabulary.',

This change was useless

@tim.plunkett:
1) good idea to replace all _sort helpers in core, with pointed pattern, but I think we need a separate issue for that.
2) yes I really wait for #1785974: Move ConfigEntity into a Core component
3) other way? the purpose to get rid of serial IDs... But we can change all vid to ->id() but I thinnk this just gives a performance regression

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -31,13 +31,14 @@ class VocabularyFormController extends EntityFormController {
-    $form['machine_name'] = array(
+    $form['vid'] = array(
       '#type' => 'machine_name',
-      '#default_value' => $vocabulary->machine_name,
+      '#default_value' => $vocabulary->vid,
       '#maxlength' => 255,
       '#machine_name' => array(
-        'exists' => 'taxonomy_vocabulary_machine_name_load',
+        'exists' => 'taxonomy_vocabulary_load',
       ),
+      '#disabled' => !$vocabulary->isNew(),

This debatable, I think we should disable change as #55

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyStorageController.phpundefined
@@ -2,55 +2,71 @@
+      // Update bundles.
+      field_attach_rename_bundle('taxonomy_term', $entity->getOriginalID(), $entity->vid);
     }
+    parent::postSave($entity, $update);
+    $this->resetCache($update ? array($entity->getOriginalID()) : array());

This is a debatable change, I think we need to call postSave() after module's logic

But we can change all vid to ->id() but I thinnk this just gives a performance regression

No, that's good API design. Using interface-defined methods instead of raw properties is what we should be doing, universally. Then what the internal property is named doesn't matter.

Unless you can show that there's more than a 10% performance difference along critical path code (which I highly doubt), don't bypass the method to access the property directly. This question illustrates one of the key reasons for that.

StatusFileSize
new1.79 KB
new83.94 KB
PASSED: [[SimpleTest]]: [MySQL] 41,497 pass(es).
[ View ]

Language upgrade for vocabularies was lost so reverted.
Ternary login changed to proposed for PHP 5.3 $a_label = $a->label() ?: '';

EDIT needs re-roll because of commited #1785974: Move ConfigEntity into a Core component

StatusFileSize
new62.69 KB
new110.35 KB
PASSED: [[SimpleTest]]: [MySQL] 41,659 pass(es).
[ View ]
new82.08 KB
PASSED: [[SimpleTest]]: [MySQL] 41,676 pass(es).
[ View ]
new5.51 KB

Just re-roll with a few fixes depending on #1785974: Move ConfigEntity into a Core component

Also here's a 2 patches:
- noid - just re-roll
- withid - an attempt to change Vocabulary->vid to Vocabulary->id() where possible, and change Term->vid to use Term->bundle()

EDIT I think that everything that lives in taxonomy module should know a database storage, PK is first thing!!!

First 2 changes are filed as separate issues
#1796604: Make constructor parameters consistent
#1796618: Change style of ternary to PHP 5.3

As per the PHP bug report (https://bugs.php.net/bug.php?id=50688) this sounds like an error/exception down the line in the sorting function which might be related to your test data related to label(). That is really the only "action" that the sort function executes on the config entity.

Here's a quick review, haven't read all of the comments, just ignore it if I'm repeating something...

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.phpundefined
@@ -34,19 +34,19 @@ class ManageFieldsTest extends FieldUiTestBase {
-      'field_name' => 'field_' . $vocabulary->machine_name,
+      'field_name' => 'field_' . $vocabulary->vid,
       'type' => 'taxonomy_term_reference',
     );
     field_create_field($field);
     $instance = array(
-      'field_name' => 'field_' . $vocabulary->machine_name,
+      'field_name' => 'field_' . $vocabulary->vid,
       'entity_type' => 'node',
       'label' => 'Tags',
       'bundle' => 'article',
@@ -109,8 +109,8 @@ class ManageFieldsTest extends FieldUiTestBase {
@@ -109,8 +109,8 @@ class ManageFieldsTest extends FieldUiTestBase {
     // Assert the field appears in the "re-use existing field" section for
     // different entity types; e.g. if a field was added in a node entity, it
     // should also appear in the 'taxonomy term' entity.
-    $vocabulary = taxonomy_vocabulary_load(1);
-    $this->drupalGet('admin/structure/taxonomy/' . $vocabulary->machine_name . '/fields');
+    $vocabulary = taxonomy_vocabulary_load('tags');
+    $this->drupalGet('admin/structure/taxonomy/' . $vocabulary->vid . '/fields');

Is there a reason these weren't replaced with id()?

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.phpundefined
@@ -343,15 +343,16 @@ class ForumTest extends WebTestBase {
+    // @todo check that no vid field availiable.
+    $new_vid = drupal_strtolower(drupal_substr($this->randomName(), 3, 9));

Typo, should be "available". Also not sure I understand this comment..

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyUnitTest.phpundefined
@@ -39,7 +39,7 @@ class VocabularyUnitTest extends TaxonomyTestBase {
-  function testTaxonomyVocabularyLoadReturnFalse() {
+  /*function testTaxonomyVocabularyLoadReturnFalse() {
     // Load a vocabulary that doesn't exist.
     $vocabularies = taxonomy_vocabulary_load_multiple();
     $vid = count($vocabularies) + 1;
@@ -54,7 +54,7 @@ class VocabularyUnitTest extends TaxonomyTestBase {
@@ -54,7 +54,7 @@ class VocabularyUnitTest extends TaxonomyTestBase {
     $vocabulary = taxonomy_vocabulary_load($vid);
     $this->assertTrue(!empty($vocabulary) && is_object($vocabulary), 'Vocabulary is an object.');
     $this->assertEqual($vocabulary->vid, $vid, 'Valid vocabulary vid is the same as our previously invalid one.');
-  }

Commented out test method. Can this be removed or why is this?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyUnitTest.phpundefined
@@ -139,33 +139,36 @@ class VocabularyUnitTest extends TaxonomyTestBase {
     // Fetch vocabulary 1 by name.
-    $vocabulary = current(entity_load_multiple_by_properties('taxonomy_vocabulary', array('name' => $vocabulary1->name)));
-    $this->assertEqual($vocabulary->vid, $vocabulary1->vid, 'Vocabulary loaded successfully by name.');
+    // @todo Commented for EntityStorageControllerInterface::loadByProperties()
+    //$vocabulary = current(entity_load_multiple_by_properties('taxonomy_vocabulary', array('name' => $vocabulary1->name)));
+    //$this->assertEqual($vocabulary->id(), $vocabulary1->id(), 'Vocabulary loaded successfully by name.');
     // Fetch vocabulary 1 by name and ID.
-    $this->assertEqual(current(taxonomy_vocabulary_load_multiple(array($vocabulary1->vid), array('name' => $vocabulary1->name)))->vid, $vocabulary1->vid, 'Vocabulary loaded successfully by name and ID.');
+    // @todo Commented for EntityStorageControllerInterface::loadByProperties()
+    //$this->assertEqual(current(taxonomy_vocabulary_load_multiple(array($vocabulary1->id()), array('name' => $vocabulary1->name)))->id(), $vocabulary1->id(), 'Vocabulary loaded successfully by name and ID.');

More outcommented stuff, haven't read through the whole issue, what's the reason, do we want to commit it like this?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyUnitTest.phpundefined
@@ -176,24 +179,26 @@ class VocabularyUnitTest extends TaxonomyTestBase {
     // Check that entity bundles are properly updated.
     $info = entity_get_info('taxonomy_term');
+    debug($info);
+    debug($this->vocabulary);
     $this->assertFalse(isset($info['bundles'][$old_name]), 'The old bundle name does not appear in entity_get_info().');

debug statements.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -31,13 +31,14 @@ class VocabularyFormController extends EntityFormController {
       '#type' => 'machine_name',
-      '#default_value' => $vocabulary->machine_name,
+      '#default_value' => $vocabulary->id(),

Just like the labels, I think this place should not use the id() method. It explicitly wants to edit the vid.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -88,13 +85,13 @@ class VocabularyFormController extends EntityFormController {
-    // During the deletion there is no 'machine_name' key.
-    if (isset($form_state['values']['machine_name'])) {
+    // During the deletion there is no 'vid' key.
+    if (isset($form_state['values']['vid'])) {
       // Do not allow machine names to conflict with taxonomy path arguments.
-      $machine_name = $form_state['values']['machine_name'];
+      $vid = $form_state['values']['vid'];
       $disallowed = array('add', 'list');
-      if (in_array($machine_name, $disallowed)) {
-        form_set_error('machine_name', t('The machine-readable name cannot be "add" or "list".'));
+      if (in_array($vid, $disallowed)) {
+        form_set_error('vid', t('The machine-readable name cannot be "add" or "list".'));

Is that comment/code still correct? vid is always set, isn't it?

Issue tags:-Configurables
StatusFileSize
new8.08 KB
new110.07 KB
PASSED: [[SimpleTest]]: [MySQL] 41,860 pass(es).
[ View ]

uasort() throws warning because no language_manager registered in drupal_container()
The first call to ::label() calls entity_get_info() which tries to find a language_manager... warning is generated in

<?php
// core\vendor\symfony\dependency-injection\Symfony\Component\DependencyInjection\Container.php
   
public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE)
    {
       
$id = strtolower($id);
        if (isset(
$this->services[$id])) {
            return
$this->services[$id];
        }
        if (isset(
$this->loading[$id])) {
            throw new
ServiceCircularReferenceException($id, array_keys($this->loading));
        }
?>

ServiceCircularReferenceException is actually what described in https://bugs.php.net/bug.php?id=50688

About commented out tests:
testTaxonomyVocabularyLoadReturnFalse() now removed because vid is no more serial.

testTaxonomyVocabularyChangeMachineName is debatable because machine name change probably could be allowed...

Other asserts are commented out while there's no implementation for EntityStorageControllerInterface::loadByProperties()

About usage of ::vid and id() I think all vocabulary units should know it's fields/properties also using label() could give translated value of the label

Issue tags:+Configurables

Tagging.

Issue tags:+Configurables

Config entities should use enforceIsNew or different way to ensure that they are not saved, details #1588422-139: Convert contact categories to configuration system

Status:Needs review» Needs work

I didn't have time to review the entirety of the latest patch since I last worked on it and don't have time right now.

So just this for now:

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -33,7 +33,7 @@ class VocabularyFormController extends EntityFormController {
-      '#default_value' => $vocabulary->id(),
+      '#default_value' => $vocabulary->vid,
@@ -125,18 +125,18 @@ class VocabularyFormController extends EntityFormController {
-        watchdog('taxonomy', 'Created new vocabulary %name.', array('%name' => $vocabulary->name), WATCHDOG_NOTICE, l(t('edit'), 'admin/structure/taxonomy/' . $vocabulary->id() . '/edit'));
+        watchdog('taxonomy', 'Created new vocabulary %name.', array('%name' => $vocabulary->name), WATCHDOG_NOTICE, l(t('edit'), 'admin/structure/taxonomy/' . $vocabulary->vid . '/edit'));
...
-        watchdog('taxonomy', 'Updated vocabulary %name.', array('%name' => $vocabulary->name), WATCHDOG_NOTICE, l(t('edit'), 'admin/structure/taxonomy/' . $vocabulary->id() . '/edit'));
+        watchdog('taxonomy', 'Updated vocabulary %name.', array('%name' => $vocabulary->name), WATCHDOG_NOTICE, l(t('edit'), 'admin/structure/taxonomy/' . $vocabulary->vid . '/edit'));
...
-    $form_state['values']['vid'] = $vocabulary->id();
-    $form_state['vid'] = $vocabulary->id();
+    $form_state['values']['vid'] = $vocabulary->vid;
+    $form_state['vid'] = $vocabulary->vid;

Please revert; always use id().

Status:Needs work» Needs review
StatusFileSize
new4.39 KB
new107.39 KB
FAILED: [[SimpleTest]]: [MySQL] 41,791 pass(es), 213 fail(s), and 245 exception(s).
[ View ]

StatusFileSize
new2.55 KB
new109.35 KB
FAILED: [[SimpleTest]]: [MySQL] 41,796 pass(es), 213 fail(s), and 245 exception(s).
[ View ]

And missed hunks with schema and dependency on config module

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -555,7 +555,7 @@ function taxonomy_term_confirm_delete($form, &$form_state, Term $term) {
-    array('%title' => $term->label())),
+      array('%title' => $term->label())),
@@ -598,7 +598,7 @@ function taxonomy_vocabulary_confirm_delete($form, &$form_state, $vid) {
-    array('%title' => $vocabulary->name)),
+      array('%title' => $vocabulary->name)),
@@ -633,12 +633,12 @@ function taxonomy_vocabulary_confirm_reset_alphabetical($form, &$form_state, $vi
+      array('%title' => $vocabulary->name)),

These look indented wrong, and why is it using ->name not ->label()?

@tim.plunkett this change is right, actually it fixes indented lines :)
I think we should cleanup name<->label() in follow-up

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-78.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.89 KB
new109.31 KB
PASSED: [[SimpleTest]]: [MySQL] 42,080 pass(es).
[ View ]

38 files changed, 374 insertions(+), 502 deletions(-)

StatusFileSize
new3.32 KB
new108.46 KB
PASSED: [[SimpleTest]]: [MySQL] 42,110 pass(es).
[ View ]

Finally uncommented tests that were broken
Fixed vocabulary name change (cache invalidation and stupid usage of $update variable)

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new117.51 KB
PASSED: [[SimpleTest]]: [MySQL] 42,792 pass(es).
[ View ]
new11.12 KB

StatusFileSize
new1.27 KB
new117.52 KB
PASSED: [[SimpleTest]]: [MySQL] 42,791 pass(es).
[ View ]

Changed name => label() as #79

Patch is growing, and requires reviews

StatusFileSize
new117.56 KB
FAILED: [[SimpleTest]]: [MySQL] 46,094 pass(es), 5 fail(s), and 8 exception(s).
[ View ]
new6.56 KB

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-87.patch, failed testing.

Status:Needs work» Needs review

Views in core now requires to fix it's plugins and probably tests.

Status:Needs review» Needs work

It probably has integration for the DB table and columns.

Status:Needs work» Needs review
StatusFileSize
new142.84 KB
FAILED: [[SimpleTest]]: [MySQL] 46,195 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new23.74 KB

Not sure how to remove vocabulary table, but filter on vocabulary works for term view

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-90.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new154.35 KB
PASSED: [[SimpleTest]]: [MySQL] 46,174 pass(es).
[ View ]
new37.62 KB

Should pass tests

Filed #1821274: Add back ability to sort on vocabulary weight and name
Dropped legacy staff and renamed vocabulary all over to vid

andypost: dawehner: what to do with old/legacy "vids" staff? I cant change 'vocabularies' to vids
andypost: also it means that I need to convert 'vocabularies to vids back
dawehner: andypost: oh ... this code should have been removed a while ago
dawehner: andypost: so just remove that ... upgrade path will be done via CMI

Just a small review of the views changes. In general that's great work!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_default/Tid.phpundefined
@@ -109,7 +99,7 @@ public function buildOptionsForm(&$form, &$form_state) {
+    $options['vids'] = array_filter($options['vocabularies']);

This doesn't look right, it should be vids

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.phpundefined
@@ -28,17 +28,8 @@ class TaxonomyIndexTid extends ManyToOne {
-    if (!empty($this->definition['vocabulary'])) {
-      $this->options['vocabulary'] = $this->definition['vocabulary'];
-    }
-
-    // Convert legacy vid option to machine name vocabulary.
-    if (isset($this->options['vid']) && !empty($this->options['vid']) & empty($this->options['vocabulary'])) {
-      $vocabularies = taxonomy_vocabulary_get_names();
-      $vid = $this->options['vid'];
-      if (isset($vocabularies[$vid], $vocabularies[$vid]->machine_name)) {
-        $this->options['vocabulary'] = $vocabularies[$vid]->machine_name;
-      }
+    if (!empty($this->definition['vid'])) {
+      $this->options['vid'] = $this->definition['vid'];

I really like this changes!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.phpundefined
@@ -59,26 +50,26 @@ protected function defineOptions() {
-      if (empty($this->definition['vocabulary'])) {
...
+      if (empty($this->definition['vid'])) {

There should be some changes on the taxonomy.views.inc as well. I guess using vocabulary as a code flag sounds legal.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.phpundefined
@@ -152,15 +143,13 @@ function value_form(&$form, &$form_state) {
+        // @todo Sorting on vocabulary properties http://drupal.org/node/1821274

Good idea to add a @todo!

StatusFileSize
new154.52 KB
PASSED: [[SimpleTest]]: [MySQL] 46,177 pass(es).
[ View ]
new2.29 KB

Fixed #94

StatusFileSize
new1.09 KB
new154.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1552396-core-voc-96.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Unless of these comments i couldn't find anything in this huge patch.

+++ b/core/modules/forum/forum.moduleundefined
@@ -220,17 +220,8 @@ function forum_menu_local_tasks_alter(&$data, $router_item, $root_path) {
-    // Within hook_entity_info(), we can't invoke entity_load() as that would
-    // cause infinite recursion, so we call taxonomy_vocabulary_get_names()

It is actually great that we don't have this dependency anymore.

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.phpundefined
@@ -328,15 +328,16 @@ function editForumTaxonomy() {
+    // @todo check that no vid form-field available.
+    $new_vid = drupal_strtolower(drupal_substr($this->randomName(), 3, 9));

I don't actually see this variable used, is this by intention?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyStorageController.phpundefined
@@ -76,14 +90,17 @@ protected function postDelete($entities) {
+    drupal_static_reset('entity_get_info');

Just in general it might make sense to move this to the upper class once.

+++ b/core/profiles/standard/standard.infoundefined
@@ -6,6 +6,7 @@ dependencies[] = node
+dependencies[] = config

Isn't the config module about the admin interface, so we don't need this in the standard profile?

#96: 1552396-core-voc-96.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +VDC, +Configurables

The last submitted patch, 1552396-core-voc-96.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new153.7 KB
FAILED: [[SimpleTest]]: [MySQL] 46,397 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Rerolled for both the entity type plugins and EFQv2.

Status:Needs review» Needs work

The last submitted patch, vocab-1552396-100.patch, failed testing.

@tim.plunkett please provide interdiff to follow your changes

You can't provide interdiffs when doing rerolls. There were no changes made. You can diff the patches if you like.

Status:Needs work» Needs review
StatusFileSize
new153.04 KB
FAILED: [[SimpleTest]]: [MySQL] 47,550 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -109,12 +109,13 @@ function taxonomy_permission() {
function taxonomy_entity_info_alter(&$info) {
-  foreach (taxonomy_vocabulary_get_names() as $machine_name => $vocabulary) {
-    $info['taxonomy_term']['bundles'][$machine_name] = array(
-      'label' => $vocabulary->name,
+  foreach (taxonomy_vocabulary_get_names() as $id) {
+    $config = config('taxonomy.vocabulary.' . $id);
+    $return['taxonomy_term']['bundles'][$id] = array(

function taxonomy_entity_info_alter(&$info) {
-    $return['taxonomy_term']['bundles'][$id] = array(
+    $info['taxonomy_term']['bundles'][$id] = array(

Other changes #97:
- removed config.module from standard profile
- $new_vid removed as unused variable

I don't think that drupal_static_reset('entity_get_info'); should be moved in this patch

Status:Needs review» Needs work
Issue tags:-Configuration system, -D8MI, -VDC, -Configurables

The last submitted patch, 1552396-core-voc-104.patch, failed testing.

Status:Needs work» Needs review

#104: 1552396-core-voc-104.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +VDC, +Configurables

The last submitted patch, 1552396-core-voc-104.patch, failed testing.

Status:Needs work» Needs review

Now it's green!

#104: 1552396-core-voc-104.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +VDC, +Configurables

The last submitted patch, 1552396-core-voc-104.patch, failed testing.

StatusFileSize
new5.05 KB
new157.08 KB
PASSED: [[SimpleTest]]: [MySQL] 47,727 pass(es).
[ View ]

Entity translation test is broken somehow because translation_entity_entity_insert() trying to insert into translation entity but vid is not integer.

Patch introduces a bit of clean-ups and tries to fix the new test

PS: current diffstat 55 files changed, 485 insertions(+), 824 deletions(-)

Status:Needs work» Needs review

I've hit that Entity translation installation as well on the field api cmi conversion patch - see http://drupal.org/node/1735118#comment-6689412 - I had to change the entity_id to column because the field instance doesn't haven id anymore, but a uuid, this might be the case here as well.

@swentel I think better replace entity_id with entity_uuid - related #1726734: Replace serial entity IDs with UUIDs in URLs, or even globally?

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-111.patch, failed testing.

@andypost:

@swentel is right: the UI test failure is caused by a wrong schema definition, as we didn't take into account config entities. Moreover #1832000: Entity translation metadata should be stored only for translation enabled entities/bundles. Sorry for getting in the way :(

I think the same fix proposed for the Fields->CMI issue should be ok. The other test failures look unrelated to me: that test should rely only on the test entity, the test storage controller and efq. No idea of what is happening there.

Triggering a retest, as I could not reproduce the failures here since the drupal installation is failing.

Status:Needs work» Needs review
Issue tags:-Configuration system, -D8MI, -VDC, -Configurables

#111: 1552396-core-voc-111.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +VDC, +Configurables

The last submitted patch, 1552396-core-voc-111.patch, failed testing.

I've confirmed that with my patch in #1832000: Entity translation metadata should be stored only for translation enabled entities/bundles the tests are green. Can we get a quick RTBC over there to unblock this?

#1832932: translation_entity_entity_insert() assumes entity IDs are integers was commited and should also unblock this. Let's try a re-test.

Status:Needs work» Needs review
Issue tags:-Configuration system, -D8MI, -VDC, -Configurables

#111: 1552396-core-voc-111.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-111.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Configuration system, +D8MI, +VDC, +Configurables

#111: 1552396-core-voc-111.patch queued for re-testing.

Test bot comes back green on #111 from the recent re-queueing.

Reading back a bunch of comments, this looks like it's been reviewed and might be RTBC. @andypost do you agree? or would you like a final look at #111? Maybe by @dawehner or someone already familiar with this...

Any feelings about what is needed to get this committed?
I thought updating the issue summary might help
and posting a version of the patch with just the tests and no fix to show it fails the test bot.

StatusFileSize
new157.08 KB
PASSED: [[SimpleTest]]: [MySQL] 47,986 pass(es).
[ View ]

Chasing head. no changes

@YesCT I think patch RTBC but suppose still needs review from @sun, the whole thing should be commited and issue summary is enough - we are converting vocabularies as part of #1802750: [Meta] Convert configurable data to ConfigEntity system

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-124.patch, failed testing.

Status:Needs work» Needs review

The bots sometimes fail to write config files. So retest helps

#124: 1552396-core-voc-124.patch queued for re-testing.

StatusFileSize
new157.06 KB
PASSED: [[SimpleTest]]: [MySQL] 48,195 pass(es).
[ View ]

merge head

Wow, this is an amazing patch. I could only find two small things:

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyStorageController.php
@@ -2,55 +2,71 @@
-   * Overrides Drupal\Core\Entity\DatabaseStorageController::postSave().
+   * Overrides Drupal\config\ConfigStorageController::postSave().

The class name is actually 'Drupal\Core\Config\Entity\ConfigStorageController'.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -700,14 +677,14 @@ function taxonomy_term_load_parents_all($tid) {
+function taxonomy_term_load_children($tid, $bundle = 0) {

$bundle should default to NULL.

Otherwise, this looks RTBC to me.

StatusFileSize
new157.08 KB
PASSED: [[SimpleTest]]: [MySQL] 48,313 pass(es).
[ View ]
new3.65 KB

Thanx for review. Fixed
Also I found that merge caused to exclude removal of views handlers

Ugh, sorry, I forgot to say that first issue must be fixed throught that file, it wasn't only one occurence.

StatusFileSize
new157.11 KB
PASSED: [[SimpleTest]]: [MySQL] 48,313 pass(es).
[ View ]
new1.46 KB

Thanx a lot

StatusFileSize
new160.23 KB
PASSED: [[SimpleTest]]: [MySQL] 48,314 pass(es).
[ View ]
new7.92 KB

Found a couple more places where we needed to use id() and bundle(). Since we have issues for the todo's introduce by this patch, I'll RTBC if the tests come back green.

Assigned:sun» Unassigned

This looks excellent.

You've clearly taken this issue over and it very much looks like we're very close to completion here.

I only have a few relatively minor issues, which are (hopefully) easy to address:

+++ b/core/modules/forum/forum.module
@@ -596,7 +587,8 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
function forum_form_taxonomy_vocabulary_form_alter(&$form, &$form_state, $form_id) {
...
+  $vocabulary = $form_state['controller']->getEntity($form_state);
+  if (!$vocabulary->isNew() && $vid == $vocabulary->id()) {

Off-hand, I do not understand why this condition is limited to an existing 'forum' vocabulary — if Forum module is installed, then the configured vocabulary ID, regardless of whether it already exists or not, should get the appropriate treatment.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.php
@@ -152,15 +143,13 @@ function value_form(&$form, &$form_state) {
         $query = db_select('taxonomy_term_data', 'td');
-        $query->innerJoin('taxonomy_vocabulary', 'tv', 'td.vid = tv.vid');
         $query->fields('td');
-        $query->orderby('tv.weight');
-        $query->orderby('tv.name');
+        // @todo Sorting on vocabulary properties http://drupal.org/node/1821274

TBH, I have no idea why anyone would ever want to sort taxonomy terms by the weights of their associated vocabularies. The weight of vocabularies has a single and concrete purpose only; it controls the order of vocabularies if they appear in e.g. select form elements on administration pages and perhaps also node/add or node/%/edit.

Speaking of, is there are actually any remaining case in which vocabularies appear in that way...? ;)

In short, I'd recommend to remove this @todo and the sort option, without replacement, and close that issue as won't fix.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyUnitTest.php
@@ -139,28 +118,31 @@ function testTaxonomyVocabularyLoadMultiple() {
     // Fetch vocabulary 1 by name.
-    $vocabulary = current(entity_load_multiple_by_properties('taxonomy_vocabulary', array('name' => $vocabulary1->name)));
-    $this->assertEqual($vocabulary->vid, $vocabulary1->vid, 'Vocabulary loaded successfully by name.');
+    // @todo Commented for EntityStorageControllerInterface::loadByProperties()
+    //$vocabulary = current(entity_load_multiple_by_properties('taxonomy_vocabulary', array('name' => $vocabulary1->name)));
+    //$this->assertEqual($vocabulary->id(), $vocabulary1->id(), 'Vocabulary loaded successfully by name.');
     // Fetch vocabulary 1 by name and ID.
-    $this->assertEqual(current(taxonomy_vocabulary_load_multiple(array($vocabulary1->vid), array('name' => $vocabulary1->name)))->vid, $vocabulary1->vid, 'Vocabulary loaded successfully by name and ID.');
+    // @todo Commented for EntityStorageControllerInterface::loadByProperties()
+    //$this->assertEqual(current(taxonomy_vocabulary_load_multiple(array($vocabulary1->id()), array('name' => $vocabulary1->name)))->id(), $vocabulary1->id(), 'Vocabulary loaded successfully by name and ID.');

mmm, I do not understand these @todos and why the assertions have been commented out.

Ah. loadByProperties() isn't supported for config entities, since there is no EFQ support for config entities yet, I see.

Gotcha. Let's simply remove those assertions, please. If that support will be added, we will have dedicated tests for config entities. No need to duplicate those assertions here, and actually, it's guaranteed that no one will remember that we've commented out these lines here. Thus, /dev/null those lines.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -31,14 +31,15 @@ public function form(array $form, array &$form_state, EntityInterface $vocabular
+      '#disabled' => !$vocabulary->isNew(),

Hmmm... It was possible to rename vocabulary machine names before. The entire infrastructure for that is actually in place alraedy, so I don't really see the reason for why we'd suddenly want to disallow that?

+++ b/core/modules/taxonomy/taxonomy.install
@@ -13,9 +15,10 @@ function taxonomy_uninstall() {
+  $config_names = config_get_storage_names_with_prefix('taxonomy.vocabulary.');
+  foreach ($config_names as $config_name) {
+    $vid = substr($config_name, strlen('taxonomy.vocabulary.'));

$vid will blow up as soon as there might be additional config objects that share the vocabulary ID, but have an additional suffix; e.g., taxonomy.vocabulary.ID.blah

I guess it's OK to go with this for now since we generally need to correct all code that attempts to extract the config entity ID from a config object name, but I wanted to make sure to mention that. The correct code involves some fancy explode() trickery on both the config_prefix and the $config_name, so yeah, let's just simply leave that for some other issue.

+++ b/core/modules/taxonomy/taxonomy.install
@@ -23,68 +26,6 @@ function taxonomy_uninstall() {
-      'machine_name' => array(
-        'type' => 'varchar',
-        'length' => 255,
@@ -101,11 +42,11 @@ function taxonomy_schema() {
       'vid' => array(
...
+        'type' => 'varchar',
+        'length' => 255,

Ouch. Did we really allow 255 chars for a vocabulary machine name in D7...? That is going to bite us, badly. :-/

Anyway, that's edge-case stuff to be figured out later.

We should, however, create a follow-up issue to limit the maximum length of the new vocabulary IDs to 64 or so.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -700,14 +677,14 @@ function taxonomy_term_load_parents_all($tid) {
- * @param $vid
+ * @param $bundle
...
-function taxonomy_term_load_children($tid, $vid = 0) {
+function taxonomy_term_load_children($tid, $bundle = NULL) {
@@ -715,8 +692,8 @@ function taxonomy_term_load_children($tid, $vid = 0) {
-    if ($vid) {
-      $query->condition('t.vid', $vid);
+    if ($bundle) {
+      $query->condition('t.vid', $bundle);
@@ -731,7 +708,7 @@ function taxonomy_term_load_children($tid, $vid = 0) {
- * @param $vid
+ * @param $bundle
@@ -750,17 +727,17 @@ function taxonomy_term_load_children($tid, $vid = 0) {
-function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities = FALSE) {
+function taxonomy_get_tree($bundle, $parent = 0, $max_depth = NULL, $load_entities = FALSE) {
...
-  if (!isset($children[$vid])) {
-    $children[$vid] = array();
-    $parents[$vid] = array();
-    $terms[$vid] = array();
+  if (!isset($children[$bundle])) {
+    $children[$bundle] = array();
+    $parents[$bundle] = array();
+    $terms[$bundle] = array();
@@ -977,11 +947,11 @@ function _taxonomy_get_tid_from_term(Term $term) {
-function taxonomy_implode_tags($tags, $vid = NULL) {
+function taxonomy_implode_tags($tags, $bundle = NULL) {

(and continuing) These changes look unnecessary and actually invalid to me — the argument is still a vocabulary ID ("vid") and we're still limiting results on that vid.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -916,36 +893,29 @@ function taxonomy_vocabulary_load_multiple(array $vids = NULL) {
+function taxonomy_vocabulary_sort(array &$vocabularies = array()) {
+  // @todo Remove error suppressing when http://drupal.org/node/1799600 is
+  // fixed.
+  @uasort($vocabularies, 'Drupal\Core\Config\Entity\ConfigEntityBase::sort');

This actually sounds like a blocker to me — I still do not really understand the bug being reported in the referenced issue, and I also do not understand how there could be a vocabulary without a weight property (which in turn would trigger the PHP notice that is being claimed).

That said, I'll try to look into that issue once more to see whether I can make any more sense of it today. If you have any additional clarifications you can share over there, that would be helpful and appreciated.

Reviewed the patch from the point of view of the multilingual features. I was delighted to find language support intact which is key. Good stuff :)

StatusFileSize
new9.61 KB
new154.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1552396-core-voc-136.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new154.29 KB
PASSED: [[SimpleTest]]: [MySQL] 48,549 pass(es).
[ View ]

#134
1) forum_form_taxonomy_vocabulary_form_alter -fixed
2) vocabulary sorting - could be useful to group/order terms - as is
3) loadByProperties() - removed commented tests
4) machine name change - probably it work for now, so removed #disable
5) bundle changed back to $vid
6) taxonomy_vocabulary_sort() - added patch without suppressing error, let's see (actually the problem in language manager)

EDIT: Locally I got broken 1 test "taxonomy vocabulary interface" because of warning
Drupal\taxonomy\Tests\VocabularyTest->testTaxonomyAdminChangingWeights()

taxonomy_vocabulary_sort()

uasort() [function.uasort]: Array was modified by the user comparison function
uasort(Array, 'Drupal\Core\Config\Entity\ConfigEntityBase::sort')
taxonomy_vocabulary_sort(Array)
Drupal\taxonomy\Tests\VocabularyTest->testTaxonomyAdminChangingWeights()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '1', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page()
system_batch_page()
call_user_func_array('system_batch_page', Array)
Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Symfony\Component\HttpKernel\Kernel->handle(Object)

Status:Needs review» Needs work
Issue tags:-Configuration system, -D8MI, -VDC, -Configurables

The last submitted patch, 1552396-core-voc-136.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Configuration system, +D8MI, +VDC, +Configurables

#136: 1552396-core-voc-136.patch queued for re-testing.

the second patch fails to apply, locally and by the test bot
http://qa.drupal.org/pifr/test/390183
looks like a lot of file locations changed.

Status:Needs review» Needs work
StatusFileSize
new154.27 KB
FAILED: [[SimpleTest]]: [MySQL] 48,647 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new154.27 KB
FAILED: [[SimpleTest]]: [MySQL] 48,670 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

rerolling I had this conflict:

  /**
   * Overrides \Drupal\translation_entity\Tests\EntityTranslationUITest::createEntity().
   */
<<<<<<< HEAD
  protected function createEntity($values, $langcode, $vocabulary_name = NULL) {
    if (!isset($vocabulary_name)) {
      $vocabulary_name = $this->bundle;
    }
    $vocabulary = taxonomy_vocabulary_machine_name_load($vocabulary_name);
=======
  protected function createEntity($values, $langcode) {
    $vocabulary = entity_load('taxonomy_vocabulary', $this->bundle);
>>>>>>> 136b
    $values['vid'] = $vocabulary->id();
    return parent::createEntity($values, $langcode);
  }

in core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTranslationUITest.php

I merged it as:

  protected function createEntity($values, $langcode, $vocabulary_name = NULL) {
    if (!isset($vocabulary_name)) {
      $vocabulary_name = $this->bundle;
    }
    $vocabulary = entity_load('taxonomy_vocabulary', $vocabulary_name);
    $values['vid'] = $vocabulary->id();
    return parent::createEntity($values, $langcode);
  }

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-140.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.93 KB
new158.04 KB
PASSED: [[SimpleTest]]: [MySQL] 48,790 pass(es).
[ View ]

I filed #1848904: Bundles cannot be specified in Entity Translation tests

Here's a fixed test that includes fix from issue above

@andypost this one passes the testbot. the patch in #1848904-6: Bundles cannot be specified in Entity Translation tests (patch 5) failed tests. Which version of the other patch does this patch include?

@YesCT #143 patch is different from #1848904-13: Bundles cannot be specified in Entity Translation tests because after conversion term needs just a bundle

StatusFileSize
new158.8 KB
PASSED: [[SimpleTest]]: [MySQL] 48,729 pass(es).
[ View ]
new2.47 KB

Status:Needs review» Needs work
Issue tags:-Configuration system, -D8MI, -VDC, -Configurables

The last submitted patch, 1552396-core-voc-146.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Configuration system, +D8MI, +VDC, +Configurables

#146: 1552396-core-voc-146.patch queued for re-testing.

StatusFileSize
new156.41 KB
FAILED: [[SimpleTest]]: [MySQL] 48,414 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-149.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new158.72 KB
PASSED: [[SimpleTest]]: [MySQL] 48,796 pass(es).
[ View ]
new2.86 KB

Fixed broken tests

Can we mark this rtbc? I couldn't find any issues but I'm not sure all the things mentioned in #134 are fixed.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.phpundefined
@@ -152,15 +143,13 @@ function value_form(&$form, &$form_state) {
-        $query->orderby('tv.weight');
-        $query->orderby('tv.name');
+        // @todo Sorting on vocabulary properties http://drupal.org/node/1821274

The only thing from #134 still there

And

limit the maximum length of the new vocabulary IDs to 64

I filed #1853304: Limit the maximum length of the new vocabulary IDs to 64

Status:Needs review» Reviewed & tested by the community

I reviewed this, and it looks sound. taxonomy_vocabulary_sort() is the only ugly part of this, and there is a @todo with an issue (#1799600: Add test of sorting for configuration entities).

Let's get this in!

I also took a look and everything looks great, beside the already noted sorting todo which already has a follow-up!

The only minor and obviously not commit-blocking point I found:

@@ -1424,7 +1394,7 @@ function taxonomy_node_insert(Node $node) {
- * @param Drupal\node\Node $node
+ * @param Drupal\node\Plugin\Core\Entity\Node $node

Should have a leading \ at the namespace (if changed already...). Others also.

StatusFileSize
new158.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1552396-core-voc-156.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.31 KB

Status:Reviewed & tested by the community» Needs work

I only found minor issues, but I think the dropping of the taxonomy_vocabulary from existing sites is a blocker - some contrib modules or custom code might still reference vid and it costs us nothing to keep the table around until 9.x.

TBH, I have no idea why anyone would ever want to sort taxonomy terms by the weights of their associated vocabularies. The weight of vocabularies has a single and concrete purpose only; it controls the order of vocabularies if they appear in e.g. select form elements on administration pages and perhaps also node/add or node/%/edit.

This pre-dates the taxonomy term reference field. When preparing $node->taxonomy, taxonomy module would query all terms for the node, then order them by vocabulary weight first, then term weight, then name (and it was a horrible query too). I was surprised to see support for this in core any longer at all, but it looks like Views trying to support Drupal 6-style term rendering for field displays..

.

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -349,3 +277,50 @@ function taxonomy_update_8003(&$sandbox) {
+    $config = config('taxonomy.vocabulary.' . $vocabulary->machine_name)
+      ->set('vid', $vocabulary->machine_name)
+      ->set('name', $vocabulary->name)
+      ->set('uuid', !empty($vocabulary->uuid) ? $vocabulary->uuid : $uuid->generate())
+      ->set('description', $vocabulary->description)
+      ->set('hierarchy', $vocabulary->hierarchy)
+      ->set('weight', $vocabulary->weight)
+      ->set('langcode', $vocabulary->langcode)
+      ->save();

We're relying on the configuration storage never changing during the entirety of the Drupal 8 cycle by using config() here. That's not introduced in this patch by any means, but seeing it again I've opened #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage.

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -349,3 +277,50 @@ function taxonomy_update_8003(&$sandbox) {
+  $map = db_query('SELECT vid, machine_name FROM {taxonomy_vocabulary}')->fetchAllKeyed();
+  foreach ($map as $vid => $machine_name) {
+    db_update('taxonomy_term_data')
+      ->condition('vid', $vid)
+      ->fields(array('vid' => $machine_name))
+      ->execute();
+  }

This doesn't leave any historical information around for modules that were relying on vid. So can we really get away with dropping the table in the next update? I think it'd be better to leave it for existing sites then drop it in Drupal 9.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -69,7 +69,7 @@ function taxonomy_help($path, $arg) {
-      $vocabulary = taxonomy_vocabulary_machine_name_load($arg[3]);

So, so happy to see that horrible function (which I added) gone.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -625,18 +601,19 @@ function taxonomy_vocabulary_static_reset(array $ids = NULL) {
+    $config_names = config_get_storage_names_with_prefix('taxonomy.vocabulary.');
+    foreach ($config_names as $config_name) {
+      $id = substr($config_name, strlen('taxonomy.vocabulary.'));
+      $names[$id] = $id;
+    }

This seems like it'll be needed a lot with prefixed config names - maybe a follow-up to add an additional helper?

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -916,36 +893,29 @@ function taxonomy_vocabulary_load_multiple(array $vids = NULL) {
+function taxonomy_vocabulary_sort(array &$vocabularies = array()) {
+  // @todo Remove error suppressing when http://drupal.org/node/1799600 is
+  // fixed.
+  @uasort($vocabularies, 'Drupal\Core\Config\Entity\ConfigEntityBase::sort');

It looks like sun's feedback here wasn't really answered. Do we actually need to order taxonomy vocabularies in the UI at all any more? fine with removing that in a follow-up.

+++ b/core/modules/taxonomy/taxonomy.views.incundefined
@@ -13,88 +13,6 @@
function taxonomy_views_data() {
   $data = array();
-  // taxonomy_vocabulary table

bye bye :)

Status:Needs work» Needs review
Issue tags:-Configuration system, -D8MI, -VDC, -Configurables

#156: 1552396-core-voc-156.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +VDC, +Configurables

The last submitted patch, 1552396-core-voc-156.patch, failed testing.

Status:Needs work» Needs review

Pre-emptively added the vocabulary table to #1860986: Drop left-over tables from 8.x.

In trying to decipher @catch's review in #158 and briefly discussing with him in IRC, it looks like almost all of the review issues are off-topic for this particular issue and need to be handled in separate follow-up issues (which partially exist already).

The conclusion is:

1) We don't want drop the taxonomy_vocabulary table for D8. Just leave it intact. Let's simply remove that module update function.

2) One or two or three of @catch's review issues are asking for dedicated follow-up issues, which may not exist yet:

A) The Views sort handler on vocabulary weight should be dropped. Separate issue.

B) config() in module updates is covered by #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage & Co.

C) We should not change the taxonomy_vocabulary table (and replace vid with the machine name). Let's simply leave the table as-is and perform the conversion from machine_name to vid in the migration to config. This is the only part that needs to be done for this issue (next to not dropping that table).

D) Add an optional argument for config_get_storage_names_with_prefix() to return results without the passed-in $prefix. Was discussed in the past already, still a good idea. While being there, rename that ugly function to config_list_all(), thanks. Separate issue. (which should be created, but which should also not block this issue.)

E) Let's re-evaluate the need for weights on vocabularies in a follow-up issue. This issue should be created ahead of time/commit.

That's it. :)

Really, don't lose hope. We're this --><--- close to get this committed :) Thanks everyone for making this happen!

@sun thanx for "decipher" but I don't a get a part about weight - if we leave taxonomy_vocabulary table orphaned so how the weight handler should operate? current patch just drops it to pass tests

I just noticed that the Vocabulary class documents $vid as an @integer, but strings like 'tags' are actually used. I'm not sure when that inconsistency was introduced, it may have been before this patch.

StatusFileSize
new156.28 KB
FAILED: [[SimpleTest]]: [MySQL] 49,301 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.2 KB

Here's a merged head and fixes:
1) vid is string per #163
2) if we keep vocabularie's table so returned back schema definition and update hook

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-164.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new157.77 KB
PASSED: [[SimpleTest]]: [MySQL] 49,327 pass(es).
[ View ]
new1.5 KB

Fixed new efq-test

Status:Needs review» Needs work

The last submitted patch, 1552396-core-voc-166.patch, failed testing.

Status:Needs work» Needs review

Bot was fixed, now passes tests

Status:Needs review» Needs work

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -23,6 +23,68 @@ function taxonomy_uninstall() {
function taxonomy_schema() {
+  $schema['taxonomy_vocabulary'] = array(
+    'description' => 'Stores vocabulary information.',
+    'fields' => array(
+      'vid' => array(

Don't re-add the schema definition of the table. We don't want to add it for new installations, just not delete it when upgrading. People might have custom tables with a vid reference that they need to upgrade somehow. Or altered the table, or still need the data in it for some other reason...

Status:Needs work» Needs review
StatusFileSize
new159.86 KB
PASSED: [[SimpleTest]]: [MySQL] 49,329 pass(es).
[ View ]
new2.3 KB

Done

Status:Needs review» Reviewed & tested by the community

Looks good to me now.

Title:Convert vocabularies into configurationChange notice: Convert vocabularies into configuration
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

Alright. Committed/pushed to 8.x, this will need a change notice.

Yay, this is a great change making vocabularies the first config entities to have a language selector and have language configuration that applies to entities. Next up content types? #111715: Convert node/content types into configuration

Issue tags:+language-config

Kudos for the awesome work !

Just a note that the upgrade path creates taxonomy.vocabulary.*.yml entries, but not the corresponding manifest file & entries.
No biggie, all the "convert X to config entities" tasks that got in so far currently fail to do so too.
We'd need the helper function added in #1817182-12: Add upgrade path tests for contact category config conversion

Can we please file a follow-up to create tests for this/throw some kind of exception when this happens? This is at least the second or third time we've missed this.

Status:Active» Needs review

Here is a initial change notice: http://drupal.org/node/1873210

Status:Needs review» Reviewed & tested by the community

Fixed up with @andypost and @DraveRobber.

Title:Change notice: Convert vocabularies into configurationConvert vocabularies into configuration
Priority:Critical» Normal
Status:Reviewed & tested by the community» Fixed

I read through it, looks good to me.

Issue summary:View changes

Updated issue summary.

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.