Posted by sun on April 28, 2012 at 10:53pm
66 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | taxonomy.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Configurables, Configuration system, D8MI, language-config, VDC |
Issue Summary
^^
Part of meta-issues
#1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1
#1802750: [Meta] Convert configurable data to ConfigEntity system
Background Information
Config entities should use enforceIsNew or different way to ensure that they are not saved, details #1588422-139: Convert contact categories to configuration system
Related issues
#1480854: Convert operation links to '#type' => 'operations'
#1751244: Convert taxonomy module widgets to Plugin system
#1891690: Use EntityListController for vocabularies
Comments
#1
Oh my. This his a huge undertaking. Spent the past 6 hours exclusively with this...
#2
Amazing work!
#3
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.
#4
"General error: 3 Error writing file './drupaltestbotmysql/simpletest555283search_dataset.frm' (Errcode: 28)"
Looks like the testbot file system is full.
#5
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.
#6
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
#7
#1: drupal8.taxonomy-config.1.patch queued for re-testing.
#8
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 :-/
#9
+++ 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
#10
#8: config.taxonomy.8.patch queued for re-testing.
#11
The last submitted patch, config.taxonomy.8.patch, failed testing.
#12
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
#13
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.
#14
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
#15
@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.
#16
Alright, attached patch retains Drupal\taxonomy\Vocabulary, but refactors it to extend a new ConfigObjectBase+Interface.
I think this makes. a. lot. of. sense. :)
#17
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)
#18
The last submitted patch, config.taxonomy.17.patch, failed testing.
#19
Attached patch should fix most of the remaining test failures.
#20
The last submitted patch, config.taxonomy.18.patch, failed testing.
#21
#1054162: Taxonomy bundles not supported by EntityFieldQuery (followup) introduced a entity query alter hook implementation that needs to be removed here.
#22
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.
#23
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.
#24
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.
#25
How doable is it to split the new API functions and/or the image style clean-up into a separate issue?
#26
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.
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".
#27
@fago:
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. :)
#28
Thanks!! Here are my thoughts:
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.
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:
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?
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.
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.
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.
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.
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.
#29
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.
#30
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).
#31
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.
#32
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.
#33
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?
#34
Postponing on #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)
#35
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.
#36
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.
#37
Seems that #1761040: Rename Storable, Entity, and Configurable to Entity, ContentEntity, and ConfigEntity would be a place to do that.
#38
Created #1761048: Revert StorableInterface/EntityInterface separation
Fixed stale machine_name in Forum module.
Fixed missing dependency on Config module.
#39
The last submitted patch, config.taxonomy.37.patch, failed testing.
#40
Fixed Standard install profile.
#41
The last submitted patch, config.taxonomy.40.patch, failed testing.
#42
#40: config.taxonomy.40.patch queued for re-testing.
#43
The last submitted patch, config.taxonomy.40.patch, failed testing.
#44
Re-roll, with a few fixes (mostly about class names)
EDIT: filed #1789722: Fix ConfigStorageController::save
#45
The last submitted patch, 1552396-core-voc-44.patch, failed testing.
#46
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
#47
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.
#48
The last submitted patch, 1552396-core-voc-46.patch, failed testing.
#49
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.
#50
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
#51
The last submitted patch, 1552396-core-voc-50.patch, failed testing.
#52
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
#53
The VocabularyStorageController:postSave() called original:delete() before hook_update()
#54
The last submitted patch, 1552396-core-voc-53.patch, failed testing.
#55
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#56
Let's try disable vid change in form controller
#57
The last submitted patch, 1552396-core-voc-no-voc-change-56.patch, failed testing.
#58
Most of tests should be green.
we need to call resetCache() to clean-up static
taxonomy_vocabulary_get_namesCommented out a test because changing of machine name is not allowed
#59
The last submitted patch, 1552396-core-voc-58.patch, failed testing.
#60
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 functionso just prefixed with @ - as suggested in https://bugs.php.net/bug.php?id=50688Also 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();
}
?>
#61
The last submitted patch, 1552396-core-voc-60.patch, failed testing.
#62
Suppose tests should be green
#63
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?
#64
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.
#65
@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
#66
@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
#67
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.
#68
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
#69
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 changeTerm->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
#70
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.
#71
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?
#72
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
#73
Tagging.
#74
Meta issue for these: #1802750: [Meta] Convert configurable data to ConfigEntity system
#75
Config entities should use enforceIsNew or different way to ensure that they are not saved, details #1588422-139: Convert contact categories to configuration system
#76
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().
#77
So changed back and merged head after #1480854: Convert operation links to '#type' => 'operations'
#78
And missed hunks with schema and dependency on config module
#79
+++ 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()?
#80
@tim.plunkett this change is right, actually it fixes indented lines :)
I think we should cleanup name<->label() in follow-up
#81
The last submitted patch, 1552396-core-voc-78.patch, failed testing.
#82
Added changes from #1751244: Convert taxonomy module widgets to Plugin system
#83
38 files changed, 374 insertions(+), 502 deletions(-)
#84
Finally uncommented tests that were broken
Fixed vocabulary name change (cache invalidation and stupid usage of $update variable)
#85
Re-roll after #1739928: Generalize language configuration on content types to apply to terms and other entities
#86
Changed name => label() as #79
Patch is growing, and requires reviews
#87
Re-roll after #1739928: Generalize language configuration on content types to apply to terms and other entities
#88
The last submitted patch, 1552396-core-voc-87.patch, failed testing.
#89
Views in core now requires to fix it's plugins and probably tests.
#90
It probably has integration for the DB table and columns.
#91
Not sure how to remove vocabulary table, but filter on vocabulary works for term view
#92
The last submitted patch, 1552396-core-voc-90.patch, failed testing.
#93
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 vidsandypost: 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
#94
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!
#95
Fixed #94
#96
re-roll after #1820834: Change #type machine_name default 'source' element to 'label'
#97
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?
#98
#96: 1552396-core-voc-96.patch queued for re-testing.
#99
The last submitted patch, 1552396-core-voc-96.patch, failed testing.
#100
Rerolled for both the entity type plugins and EFQv2.
#101
The last submitted patch, vocab-1552396-100.patch, failed testing.
#102
@tim.plunkett please provide interdiff to follow your changes
#103
You can't provide interdiffs when doing rerolls. There were no changes made. You can diff the patches if you like.
#104
+++ 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
#105
The last submitted patch, 1552396-core-voc-104.patch, failed testing.
#106
#104: 1552396-core-voc-104.patch queued for re-testing.
#107
The last submitted patch, 1552396-core-voc-104.patch, failed testing.
#108
Now it's green!
#109
#104: 1552396-core-voc-104.patch queued for re-testing.
#110
The last submitted patch, 1552396-core-voc-104.patch, failed testing.
#111
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(-)#112
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.
#113
@swentel I think better replace entity_id with entity_uuid - related #1726734: Replace serial entity IDs with UUIDs in URLs, or even globally?
#114
The last submitted patch, 1552396-core-voc-111.patch, failed testing.
#115
@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.
#116
#111: 1552396-core-voc-111.patch queued for re-testing.
#117
The last submitted patch, 1552396-core-voc-111.patch, failed testing.
#118
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?
#119
#1832932: translation_entity_entity_insert() assumes entity IDs are integers was commited and should also unblock this. Let's try a re-test.
#120
#111: 1552396-core-voc-111.patch queued for re-testing.
#121
The last submitted patch, 1552396-core-voc-111.patch, failed testing.
#122
#111: 1552396-core-voc-111.patch queued for re-testing.
#123
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.
#124
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
#125
The last submitted patch, 1552396-core-voc-124.patch, failed testing.
#126
The bots sometimes fail to write config files. So retest helps
#127
#124: 1552396-core-voc-124.patch queued for re-testing.
#128
merge head
#129
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.
#130
Thanx for review. Fixed
Also I found that merge caused to exclude removal of views handlers
#131
Ugh, sorry, I forgot to say that first issue must be fixed throught that file, it wasn't only one occurence.
#132
Thanx a lot
#133
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.
#134
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.blahI 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.
#135
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 :)
#136
#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 functionuasort(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)
#137
The last submitted patch, 1552396-core-voc-136.patch, failed testing.
#138
#136: 1552396-core-voc-136.patch queued for re-testing.
#139
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.
#140
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);
}
#141
#142
The last submitted patch, 1552396-core-voc-140.patch, failed testing.
#143
I filed #1848904: Bundles cannot be specified in Entity Translation tests
Here's a fixed test that includes fix from issue above
#144
@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?
#145
@YesCT #143 patch is different from #1848904-13: Bundles cannot be specified in Entity Translation tests because after conversion term needs just a bundle
#146
Patch includes changes in #1848904-13: Bundles cannot be specified in Entity Translation tests
#147
The last submitted patch, 1552396-core-voc-146.patch, failed testing.
#148
#146: 1552396-core-voc-146.patch queued for re-testing.
#149
Re-roll after #1848904: Bundles cannot be specified in Entity Translation tests
#150
The last submitted patch, 1552396-core-voc-149.patch, failed testing.
#151
Fixed broken tests
#152
Can we mark this rtbc? I couldn't find any issues but I'm not sure all the things mentioned in #134 are fixed.
#153
+++ 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
I filed #1853304: Limit the maximum length of the new vocabulary IDs to 64
#154
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!
#155
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.
#156
Re-roll after #1774332: Better handling of invalid/expired cache entries
PS: also fixed #155
#157
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.
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 upgrade path.
+++ 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 :)
#158
#156: 1552396-core-voc-156.patch queued for re-testing.
#159
The last submitted patch, 1552396-core-voc-156.patch, failed testing.
#160
Pre-emptively added the vocabulary table to #1860986: Drop left-over tables from 8.x.
#161
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 upgrade path & 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 toconfig_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!
#162
@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
#163
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.
#164
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
#165
The last submitted patch, 1552396-core-voc-164.patch, failed testing.
#166
Fixed new efq-test
#167
The last submitted patch, 1552396-core-voc-166.patch, failed testing.
#168
Bot was fixed, now passes tests
#169
+++ 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...
#170
Done
#171
Looks good to me now.
#172
Alright. Committed/pushed to 8.x, this will need a change notice.
#173
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
#174
This patch seems to have broken MyISAM support
#1871032: Taxonomy module fails to install on MyISAM due to too long schema index
#175
#176
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
#177
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.
#178
Here is a initial change notice: http://drupal.org/node/1873210
#179
Fixed up with @andypost and @DraveRobber.
#180
I read through it, looks good to me.
#181
@webchick, I filed that followup the last time. ;)
#1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL
#182
Filed follow-up #1891690: Use EntityListController for vocabularies
#183
Automatically closed -- issue fixed for 2 weeks with no activity.
#184
Follow-up to convert path #1978112: Convert taxonomy admin path to follow other core entity patterns