CommentFileSizeAuthor
#170 1552396-interdiff-170.txt2.3 KBandypost
#170 1552396-core-voc-170.patch159.86 KBandypost
#166 1552396-interdiff-166.txt1.5 KBandypost
#166 1552396-core-voc-166.patch157.77 KBandypost
#164 1552396-interdiff-164.txt3.2 KBandypost
#164 1552396-core-voc-164.patch156.28 KBandypost
#156 1552396-interdiff-156.txt1.31 KBandypost
#156 1552396-core-voc-156.patch158.73 KBandypost
#151 1552396-interdiff-151.txt2.86 KBandypost
#151 1552396-core-voc-151.patch158.72 KBandypost
#149 1552396-core-voc-149.patch156.41 KBandypost
#146 1552396-interdiff-146.txt2.47 KBandypost
#146 1552396-core-voc-146.patch158.8 KBandypost
#143 1552396-core-voc-143.patch158.04 KBandypost
#143 1552396-interdiff-143.txt4.93 KBandypost
#140 1552396-core-voc-140-sort-broken.patch154.27 KBYesCT
#140 1552396-core-voc-140.patch154.27 KBYesCT
#136 1552396-core-voc-136-sort-broken.patch154.29 KBandypost
#136 1552396-core-voc-136.patch154.29 KBandypost
#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
#132 1552396-interdiff-132.txt1.46 KBandypost
#132 1552396-core-voc-132.patch157.11 KBandypost
#130 1552396-interdiff-128-130.txt3.65 KBandypost
#130 1552396-core-voc-130.patch157.08 KBandypost
#128 1552396-core-voc-128.patch157.06 KBandypost
#124 1552396-core-voc-124.patch157.08 KBandypost
#111 1552396-core-voc-111.patch157.08 KBandypost
#111 1552396-interdiff-111.txt5.05 KBandypost
#104 1552396-core-voc-104.patch153.04 KBandypost
#100 vocab-1552396-100.patch153.7 KBtim.plunkett
#96 1552396-core-voc-96.patch154.56 KBandypost
#96 1552396-interdiff-96.txt1.09 KBandypost
#95 1552396-interdiff-93vs95.txt2.29 KBandypost
#95 1552396-core-voc-95.patch154.52 KBandypost
#93 1552396-interdiff-87vs93.txt37.62 KBandypost
#93 1552396-core-voc-93.patch154.35 KBandypost
#91 1552396-interdiff-90.txt23.74 KBandypost
#91 1552396-core-voc-90.patch142.84 KBandypost
#87 1552396-interdiff-87.txt6.56 KBandypost
#87 1552396-core-voc-87.patch117.56 KBandypost
#86 1552396-core-voc-86.patch117.52 KBandypost
#86 1552396-interdiff-86.txt1.27 KBandypost
#85 1552396-interdiff-85.txt11.12 KBandypost
#85 1552396-core-voc-85.patch117.51 KBandypost
#84 1552396-core-voc-84.patch108.46 KBandypost
#84 1552396-interdiff-84.txt3.32 KBandypost
#82 1552396-core-voc-82.patch109.31 KBandypost
#82 1552396-interdiff-82.txt2.89 KBandypost
#78 1552396-core-voc-78.patch109.35 KBandypost
#78 1552396-interdiff-78.txt2.55 KBandypost
#77 1552396-core-voc-77.patch107.39 KBandypost
#77 1552396-interdiff-77.txt4.39 KBandypost
#72 1552396-core-voc-72-withid.patch110.07 KBandypost
#72 1552396-interdiff.txt8.08 KBandypost
#69 1552396-interdiff-68vs69.txt5.51 KBandypost
#69 1552396-core-voc-69-noid.patch82.08 KBandypost
#69 1552396-core-voc-69-withid.patch110.35 KBandypost
#69 1552396-id-interdiff.txt62.69 KBandypost
#68 1552396-core-voc-68.patch83.94 KBandypost
#68 1552396-interdiff-68.txt1.79 KBandypost
#62 1552396-core-voc-62.patch84.27 KBandypost
#62 1552396-interdiff-62.txt740 bytesandypost
#60 1552396-core-voc-60.patch83.9 KBandypost
#60 1552396-interdiff-60.txt7.69 KBandypost
#58 1552396-core-voc-58.patch79.87 KBandypost
#58 1552396-interdiff-58.txt4.23 KBandypost
#56 1552396-core-voc-no-voc-change-56.patch78.8 KBandypost
#56 1552396-interdiff-56.txt632 bytesandypost
#55 1552396-core-voc-55.patch78.73 KBandypost
#55 1552396-interdiff-53vs55.txt2.09 KBandypost
#53 1552396-core-voc-53.patch77.57 KBandypost
#53 1552396-core-voc-interdif-50.txt2.95 KBandypost
#50 1552396-core-voc-50.patch78.25 KBandypost
#46 1552396-core-voc-46.patch72.01 KBandypost
#46 1552396-interdiff-44vs46.txt7.84 KBandypost
#44 1552396-core-voc-44.patch66.44 KBandypost
#40 config.taxonomy.40.patch66.05 KBsun
#38 config.taxonomy.37.patch64.31 KBsun
#35 config.taxonomy.35.patch61.45 KBsun
#19 config.taxonomy.18.patch94.96 KBsun
#17 config.taxonomy.17.patch91.58 KBsun
#16 config.taxonomy.16.patch88.9 KBsun
#8 config.taxonomy.8.patch59.2 KBsun
#1 drupal8.taxonomy-config.1.patch57.3 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
57.3 KB

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

chx’s picture

Amazing work!

aturetta’s picture

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.

Berdir’s picture

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

Looks like the testbot file system is full.

joachim’s picture

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.

jthorson’s picture

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

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

sun’s picture

FileSize
59.2 KB

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

andypost’s picture

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

andypost’s picture

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.

sun’s picture

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

fago’s picture

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.

andypost’s picture

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

sun’s picture

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

sun’s picture

FileSize
88.9 KB

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

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

sun’s picture

FileSize
91.58 KB

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 tests using Standard profile to use Testing profile instead)

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
94.96 KB

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.

sun’s picture

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

Dave Reid’s picture

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.

sun’s picture

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.

Dave Reid’s picture

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.

catch’s picture

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

fago’s picture

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

sun’s picture

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).
fago’s picture

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.

xjm’s picture

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.

sun’s picture

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

sun’s picture

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.

xjm’s picture

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.

gdd’s picture

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?

sun’s picture

sun’s picture

Issue tags: +VDC
FileSize
61.45 KB

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

sun’s picture

Status: Needs work » Needs review
FileSize
64.31 KB

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.

sun’s picture

Status: Needs work » Needs review
FileSize
66.05 KB

Fixed Standard install profile.

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

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

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
66.44 KB

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.

andypost’s picture

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

Lars Toomre’s picture

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.

tim.plunkett’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
78.25 KB

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.

andypost’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
2.95 KB
77.57 KB

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.09 KB
78.73 KB

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

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
79.87 KB

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.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +D8MI
FileSize
7.69 KB
83.9 KB

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

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

andypost’s picture

Status: Needs work » Needs review
FileSize
740 bytes
84.27 KB

Suppose tests should be green

tim.plunkett’s picture

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?

Gábor Hojtsy’s picture

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.

andypost’s picture

@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

andypost’s picture

@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

Crell’s picture

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.

andypost’s picture

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

andypost’s picture

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

Gábor Hojtsy’s picture

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.

Berdir’s picture

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?

andypost’s picture

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

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

tim.plunkett’s picture

Issue tags: +Configurables

Tagging.

xjm’s picture

andypost’s picture

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

sun’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
4.39 KB
107.39 KB
andypost’s picture

And missed hunks with schema and dependency on config module

tim.plunkett’s picture

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

andypost’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
109.31 KB
andypost’s picture

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

andypost’s picture

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

andypost’s picture

Issue summary: View changes

Updated issue summary.

podarok’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

andypost’s picture

Changed name => label() as #79

Patch is growing, and requires reviews

andypost’s picture

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

Status: Needs review » Needs work

It probably has integration for the DB table and columns.

andypost’s picture

Status: Needs work » Needs review
FileSize
142.84 KB
23.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.

andypost’s picture

Status: Needs work » Needs review
FileSize
154.35 KB
37.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
dawehner’s picture

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!

andypost’s picture

Fixed #94

andypost’s picture

dawehner’s picture

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?

xjm’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
153.7 KB

Rerolled for both the entity type plugins and EFQv2.

Status: Needs review » Needs work

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

andypost’s picture

@tim.plunkett please provide interdiff to follow your changes

tim.plunkett’s picture

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

andypost’s picture

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

dagmar’s picture

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.

andypost’s picture

Status: Needs work » Needs review

Now it's green!

andypost’s picture

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

andypost’s picture

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

swentel’s picture

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.

andypost’s picture

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

plach’s picture

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

plach’s picture

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.

Berdir’s picture

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?

Berdir’s picture

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

Berdir’s picture

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.

andypost’s picture

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

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

YesCT’s picture

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.

andypost’s picture

FileSize
157.08 KB

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.

andypost’s picture

Status: Needs work » Needs review

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

andyceo’s picture

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

andypost’s picture

FileSize
157.06 KB

merge head

amateescu’s picture

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.

andypost’s picture

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

amateescu’s picture

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

andypost’s picture

Thanx a lot

amateescu’s picture

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.

sun’s picture

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.

Gábor Hojtsy’s picture

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

andypost’s picture

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

YesCT’s picture

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

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

YesCT’s picture

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.

YesCT’s picture

Status: Needs review » Needs work
FileSize
154.27 KB
154.27 KB

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);
  }
YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
4.93 KB
158.04 KB

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

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

YesCT’s picture

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

andypost’s picture

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

andypost’s picture

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

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

dagmar’s picture

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

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

andypost’s picture

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
158.72 KB
2.86 KB

Fixed broken tests

aspilicious’s picture

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

andypost’s picture

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

tim.plunkett’s picture

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!

fago’s picture

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.

andypost’s picture

catch’s picture

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

sun’s picture

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.

catch’s picture

Status: Needs work » Needs review

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

sun’s picture

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!

andypost’s picture

@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

Anonymous’s picture

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.

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
157.77 KB
1.5 KB

Fixed new efq-test

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review

Bot was fixed, now passes tests

Berdir’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
159.86 KB
2.3 KB

Done

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now.

catch’s picture

Title: Convert vocabularies into configuration » Change 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.

Gábor Hojtsy’s picture

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

patrickd’s picture

Gábor Hojtsy’s picture

Issue tags: +language-config
yched’s picture

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

webchick’s picture

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.

andypost’s picture

Status: Active » Needs review

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

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

Fixed up with @andypost and @DraveRobber.

tim.plunkett’s picture

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

I read through it, looks good to me.

xjm’s picture

xjm’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Issue summary: View changes

Updated issue summary.