Make the taxonomy entities classed objects

Not sure if it should be part of this patch, but it would be great if we can get language supported for terms (taxonomy entities) as well. Same as for nodes, so we can just copy that code hopefully :) That would unify this part of the code / behavior.

I'll try working on this.

Yeah, like Dave Reid said in #1361226: Make the file entity a classed object, I think that language handling should be a followup once the base implementation is done. We'll want to make sure we do it consistently for all the entities where it's missing.

@xjm: understood, not trying to disturb the conversion process :)

Recategorizing per #1346204-11: [meta] Drupal 8 Entity API improvements.

xjm, I assume you are not currently working on this one, are you?

Working on an initial patch...

This one should pass the taxonomy tests, let's see if there are any other test fails... maybe forum.module and stuff like that needs work.

Oh, I was actually. =/

Sorry, I actually didn't noticed that this issue was assigned to you and when I did I was already half through with the patch..

Next try, fixed some vocabulary and term creations in forum.install, trigger.text and entity_crud_hook_test.test.

Fixed forum tests and revert the class enforcements in the hooks after discussing it with fago.

The two remaining fails are during the upgrade, not sure how to fix that:

The error:

ResponseText: Fatal error: Class 'TaxonomyVocabularyController' not found in /home/berdir/Projekte/d8/core/modules/entity/entity.module on line 297
Call Stack:
0.0000 647976 1. {main}() /home/berdir/Projekte/d8/index.php:0
0.0191 4234864 2. menu_execute_active_handler() /home/berdir/Projekte/d8/index.php:21
0.0194 4313312 3. call_user_func_array() /home/berdir/Projekte/d8/core/includes/menu.inc:512
0.0194 4313680 4. system_batch_page() /home/berdir/Projekte/d8/core/includes/menu.inc:512
0.0194 4313680 5. _batch_page() /home/berdir/Projekte/d8/core/modules/system/system.admin.inc:2333
0.0194 4317880 6. _batch_do() /home/berdir/Projekte/d8/core/includes/batch.inc:80
0.0194 4317880 7. _batch_process() /home/berdir/Projekte/d8/core/includes/batch.inc:161
0.0200 4364376 8. call_user_func_array() /home/berdir/Projekte/d8/core/includes/batch.inc:284
0.0200 4364448 9. _simpletest_batch_operation() /home/berdir/Projekte/d8/core/includes/batch.inc:284
0.0286 4973584 10. DrupalTestCase->run() /home/berdir/Projekte/d8/core/modules/simpletest/simpletest.module:177
1.0979 5368760 11. BareUpgradePathTestCase->testBareUpgrade() /home/berdir/Projekte/d8/core/modules/simpletest/drupal_web_test_case.php:520
1.0979 5368760 12. UpgradePathTestCase->performUpgrade() /home/berdir/Projekte/d8/core/modules/simpletest/tests/upgrade/upgrade_bare.test:29
6.9304 9269744 13. DrupalWebTestCase->checkPermissions() /home/berdir/Projekte/d8/core/modules/simpletest/tests/upgrade/upgrade.test:308
6.9304 9270320 14. module_invoke_all() /home/berdir/Projekte/d8/core/modules/simpletest/drupal_web_test_case.php:1190
6.9342 9333176 15. call_user_func_array() /home/berdir/Projekte/d8/core/includes/module.inc:819
6.9342 9333544 16. taxonomy_permission() /home/berdir/Projekte/d8/core/includes/module.inc:819
6.9342 9334616 17. taxonomy_get_vocabularies() /home/berdir/Projekte/d8/core/modules/taxonomy/taxonomy.module:88
6.9342 9335016 18. taxonomy_vocabulary_load_multiple() /home/berdir/Projekte/d8/core/modules/taxonomy/taxonomy.module:664
6.9342 9335168 19. entity_load() /home/berdir/Projekte/d8/core/modules/taxonomy/taxonomy.module:950
6.9342 9335248 20. entity_get_controller() /home/berdir/Projekte/d8/core/modules/entity/entity.module:234

Ok just a note for myself so I don't forget these notes for final patch reviews

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -222,7 +224,7 @@ function taxonomy_form_vocabulary_submit($form, &$form_state) {
-      $form_state['redirect'] = 'admin/structure/taxonomy/' . $vocabulary->machine_name;
+  $form_state['redirect'] = 'admin/structure/taxonomy/' . $vocabulary->machine_name;

Something wrong with indentation

+++ b/core/modules/taxonomy/taxonomy.entity.incundefined
@@ -0,0 +1,278 @@
+
+  protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE) {
+    $query = parent::buildQuery($ids, $conditions, $revision_id);

We need the override statements

+++ b/core/modules/taxonomy/taxonomy.entity.incundefined
@@ -0,0 +1,278 @@
\ No newline at end of file

Add a newline

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -255,7 +255,7 @@ class TaxonomyVocabularyUnitTest extends TaxonomyWebTestCase {
+  function dtestTaxonomyVocabularyLoadStaticReset() {

all the taxonomy term tests are turned off

-1 days to next Drupal core point release.

Upsie, fixed all points except of the override docblocks, quite a few others will need that as well..

Fatal error: Class 'TaxonomyVocabularyController' not found

looks like a registry or .info file reloading problem. Berdit told me in irc that it can't be the registry so not reloading the .info is the last on my list of options. Probbably this is not the case but just writing it down.

Added docs for the class functions.

Just skimming through the patch and noticed the following... Why remove all that documentation?

  * Saves a vocabulary.
  *
  * @param $vocabulary
- *   A vocabulary object with the following properties:
- *   - vid: The ID of the vocabulary.
- *   - name: The human-readable name of the vocabulary.
- *   - machine_name: The machine name of the vocabulary.
- *   - description: (optional) The vocabulary's description.
- *   - hierarchy: The hierarchy level of the vocabulary.
- *   - module: (optional) The module altering the vocabulary.
- *   - weight: (optional) The weight of this vocabulary in relation to other
- *     vocabularies.
- *   - original: (optional) The original vocabulary object before any changes
- *     are applied.
- *   - old_machine_name: (optional) The original machine name of the
- *     vocabulary.
- *
- * @return
- *   Status constant indicating whether the vocabulary was inserted (SAVED_NEW)
- *   or updated(SAVED_UPDATED).
+ *   A TaxonomyVocabulary instance.

The properties are now documented on the class itself. Except original (old_machine_name is gone), so we could keep that.

The return should probably be kept, but then we should unify all _save() functions (if we want to keep them). I oriented myself on comment_save() which is probably not a very nice example as there was no @return documentation in D7 either.

Attached patch fixes the TaxonomyVocabularyUnitTest test failure. Includes:

- Removed debugging code
- Fixed comments
- Fixed drupal_write_record() to properly support (entity) class instance objects
- Fixed isNew() method to (re)set the ->is_new property, so preSave() and postSave() methods can rely on it.

That said, even with the latter, the isNew() method returns a bogus result. We either need a wasNew() method/property, or need to directly access the $entity->is_new property in preSave()/postSave() (instead of the method).

Incorporated feedback from #26 and fixed upgrade path tests.

To resolve the postSave() problem with isNew(), I've changed the ->isNew() method to only set the ->is_new property once; i.e., once set, it sticks for the duration of ->save(). It is unset at the end. The only "risk" involved there would be some broken code that directly adjusts the ->is_new property during the save operation, but I do not think we want or need to care about such broken code.

+++ b/core/modules/taxonomy/taxonomy.entity.inc
@@ -0,0 +1,300 @@
+ * Entity controller and class for comments.
...
+ * Defines the comment entity class.
...
+ * This extends the DrupalDefaultEntityController class. Only alteration is
...
+class TaxonomyTermController extends EntityDatabaseStorageController {

Stale comments everywhere ;)

+++ b/core/modules/taxonomy/taxonomy.entity.inc
@@ -0,0 +1,300 @@
+  public $description;

Was there any particular reason for omitting $format? I've added it in this patch.

+++ b/core/modules/taxonomy/taxonomy.entity.inc
@@ -0,0 +1,300 @@
+   * (0 = disabled, 1 = single, 2 = multiple)
...
+  public $hierarchy;

Should refer to the actual constants, not their values.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -851,7 +642,6 @@ function taxonomy_terms_static_reset() {
-  entity_get_controller('taxonomy_term')->resetCache();

@@ -862,7 +652,6 @@ function taxonomy_terms_static_reset() {
 function taxonomy_vocabulary_static_reset($ids = NULL) {
...
-  entity_get_controller('taxonomy_vocabulary')->resetCache($ids);

Additionally reverted these removals, as I believe that code that calls into the static resetters expects to have the entity caches cleared, too. We might need to discuss this for all entity types + resetters in a separate issue.

The resetCache() are already done in the save() and delete() methods, see http://api.drupal.org/api/drupal/core--modules--entity--entity.controlle... and http://api.drupal.org/api/drupal/core--modules--entity--entity.controlle.... So it should be save to delete them IMHO.

Agreed on everything else, note that I've just copied the descriptions from the hook_schema() definition.

Yes, resetCache() is already invoked within the controller operations. However, those taxonomy_*_static_reset() functions are not necessarily invoked from within the controller. They are called in tests and possibly from other places as well. It's those other calls that expect a full reset of statics.

Now it's green time to nitpick. Only found 2 small issues after quick review.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -465,46 +413,20 @@ function taxonomy_vocabulary_save($vocabulary) {
+ * Delete vocabularies.

Deletes

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -685,62 +519,19 @@ function taxonomy_term_save($term) {
+ * Delete taxonomy terms.

Deletes

I see there are other problems with the code alrdy in core. I don't care about those in this issue. Just want to be sure new docs are ok.

Fixed #32.

More comments about my own code than a review...

+++ b/core/modules/taxonomy/taxonomy.admin.inc
@@ -105,12 +105,14 @@ function theme_taxonomy_overview_vocabularies($variables) {
-function taxonomy_form_vocabulary($form, &$form_state, $edit = array()) {
+function taxonomy_form_vocabulary($form, &$form_state, $vocabulary = array()) {
   // During initial form build, add the entity to the form state for use
   // during form building and processing. During a rebuild, use what is in the
   // form state.
   if (!isset($form_state['vocabulary'])) {
-    $vocabulary = is_object($edit) ? $edit : (object) $edit;
+    if (!is_object($vocabulary) || !($vocabulary instanceof TaxonomyVocabulary)) {
+      $vocabulary = entity_create('taxonomy_vocabulary', (array) $vocabulary);
+    }

I've just hacked this together to get it working but I really dislike it.

I have no clue why the arguments defaults to an array instead of NULL. Is there a case where we could possibly pass an array into it or is this just very very old code? Should probably be cleaned up. I guess we need some sort of pattern for how to deal with new/edit forms.

+++ b/core/modules/taxonomy/taxonomy.entity.inc
@@ -0,0 +1,304 @@
+    // Prevent leading and trailing spaces in term names.
+    $entity->name = trim($entity->name);

I guess we'll see later on if this is a behavior that's worth to be unified..

+++ b/core/modules/taxonomy/taxonomy.entity.inc
@@ -0,0 +1,304 @@
+    // Initialize parent array if not set for new terms.
+    if ($entity->isNew() && !isset($entity->parent)) {
+      $entity->parent = array(0);
+    }

I did this in preSave() because there was no way to get the isNew() information in postSave() before. As you changed this, it *might* make sense to move it into postSave(), not sure. It happened technically after the insert in the old code but there was no clear separation of pre/post anyway.

+++ b/core/modules/taxonomy/taxonomy.entity.inc
@@ -0,0 +1,304 @@
+          // @todo Drop support for nested array parents?
+          foreach ($parent as $tid) {
+            $query->values(array(
+              'tid' => $entity->tid,
+              'parent' => $tid
+            ));
+          }
+        }
+        else {

I agree with fago that there should be a single, consistent way to define an entity and we should drop support for that weird nested syntax (I think I already dropped the support for $term->parent = 5), I just didn't want to do this without feedback.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -513,13 +435,13 @@ function taxonomy_vocabulary_delete($vid) {
-  if (!empty($vocabulary->old_machine_name) && $vocabulary->old_machine_name != $vocabulary->machine_name) {
+  if (!empty($vocabulary->original->machine_name) && $vocabulary->original->machine_name != $vocabulary->machine_name) {

Note that I dropped the old_machine_name stuff, in case it wasn't obvious :)

+++ b/core/modules/taxonomy/taxonomy.module
@@ -1754,7 +1486,7 @@ function taxonomy_rdf_mapping() {
-      $term = (object) $item;
+      $term = entity_create('taxonomy_term', $item);

For the record, I *really* like changes like this. It's now obvious what is happening here (creating an entity based on the values) instead of magically creating entities by casting $item/$form_state['values']to an object.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -1089,7 +1090,7 @@ class TaxonomyLoadMultipleUnitTest extends TaxonomyWebTestCase {
     // Load the terms by tid, with a condition on vid.
     $terms3 = taxonomy_term_load_multiple(array_keys($terms2), array('vid' => $vocabulary->vid));
-    $this->assertEqual($terms2, $terms3);
+    $this->assertEqual($terms2, $terms3, 'Same terms found when limiting load to vocabulary.');

This change might look weird and irrelevant. The problem is that if this check fails then Simpletest tries to generate a message automatically with the whole content of $terms2 and $terms3. This results in a string that is too long to be saved and the test throws an exception. This makes it hard to actually see what's wrong, especially with the testbot. I guess the code never failed when this code was created..

+++ b/core/modules/taxonomy/taxonomy.test
@@ -1151,7 +1152,7 @@ class TaxonomyHooksTestCase extends TaxonomyWebTestCase {
     $this->drupalPost('taxonomy/term/' . $term->tid . '/edit', $edit, t('Save'));
-    taxonomy_terms_static_reset();
+    entity_get_controller('taxonomy_term')->resetCache(array($term->tid));

This part could now probably be reverted since the static reset call does call resetCache() too. On the other side, the new code just clears what's necessary.

+    // For that the "is_new" property may be pre-set to TRUE.

This sounds odd.

I think we should cleanup the signature of taxonomy_form_vocabulary() and remove the hack while we're at it. I see no reason why $vocabulary should default to an array.

I'm sure that's very old code. When doing the entity/field conversions in D7 I found all kinds of stuff but there's plenty still in there.

Incorporated #34 and #35.

Attached patch is a format-patch for convenience, so you don't need to review the whole she-bang once more.

+ // Prevent leading and trailing spaces in term names.
+ $entity->name = trim($entity->name);

I guess we'll see later on if this is a behavior that's worth to be unified.

IMHO, this doesn't belong into the entity class though. It should be part of the form/user input handling, and at maximum, be validated on entity validation level (which does not exist at all yet).

However, leaving this alone for now.

I did this [$entity->parent initialization] in preSave() because there was no way to get the isNew() information in postSave() before. As you changed this, it *might* make sense to move it into postSave(), not sure. It happened technically after the insert in the old code but there was no clear separation of pre/post anyway.

I think the new location in preSave() is correct, as that means that hook implementations get a prepared entity for saving.

// @todo Drop support for nested array parents?

I agree with fago that there should be a single, consistent way to define an entity and we should drop support for that weird nested syntax (I think I already dropped the support for $term->parent = 5), I just didn't want to do this without feedback.

I'd like to move further tweaks to $term->parent into a separate issue. These changes are technically irrelevant for this issue and, as API changes, they really deserve an own discussion.

- $this->assertEqual($terms2, $terms3);
+ $this->assertEqual($terms2, $terms3, 'Same terms found when limiting load to vocabulary.');

This change might look weird and irrelevant. The problem is that if this check fails then Simpletest tries to generate a message automatically with the whole content of $terms2 and $terms3. This results in a string that is too long to be saved and the test throws an exception. This makes it hard to actually see what's wrong, especially with the testbot. I guess the code never failed when this code was created..

Actually, this concerned me a bit. I don't think the test never failed. The previous size of the taxonomy term stdClass was small enough to fit into an assertion message.

Now, with the entity info and all of the additional data (which is *a lot*), the mere var_export of two term entities broke the test runner in a very very weird way. (An uncaught exception is thrown deep down the line, leading to a fatal error, and what you see in the test result is a huge chunk of triple-escaped string garbage.)

We need to watch out for this and should discuss possible resolution options in a separate issue.

pounard mentioned in the main interface patch (but after it was committed), that we should avoid copying the entity info into the classes, I'd agree with that even if it's just for smaller backtraces.

Additionally cleaned up taxonomy_term_form() in the same way.

Providing an interdiff since #33 this time.

Sorry, test failure caused by recently added taxonomy test code in another core patch.

Added some type hinting as proposed in #1391694: Use type-hinting for entity-parameters

patch...

Offcourse that fails! :D Let's try this one...

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -632,30 +627,29 @@
+function taxonomy_form_term($form, &$form_state, $term = NULL, $vocabulary = NULL) {

The array stuff is gone now so should this be type hinted?

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -101,27 +101,22 @@
+function taxonomy_form_vocabulary($form, &$form_state, $vocabulary = NULL) {

Same question

-10 days to next Drupal core point release.

Created spin-off: #1401496: forum_taxonomy_term_delete() completely broken (takes $tid instead of $term entity)

Added type-hinting to all relevant taxonomy functions and hooks.

interdiff is against #41.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -248,7 +247,7 @@ function hook_taxonomy_term_view_alter(&$build) {
-  $build['#post_render'][] = 'my_module_node_post_render';
+  $build['#post_render'][] = 'my_module_taxonomy_term_post_render';

Unrelated?

Looks RTBC to me, anyone bold enough?

This patch contains some entity system and common.inc fixes that will unblock further entification work.

Yeah this is rtbc, #48 is a minor api.php fix.

Reading through this, it looked very nice, but I caught a couple things:

1. My comment in #22 regarding documentation still isn't really addressed. Sure, it's OK to remove docs from the save functions in favor of documentation on the class itself (I assume api.drupal.org will link them correctly?... or if not it can be fixed to) but as written, the current class documentation isn't a substitute for what was there before:
   • The list of optional properties isn't as complete.
   • It's missing an indication of which properties are optional and which are required when creating a new object.
   • It doesn't explain that you use entity_create() to do so.

   The previous docs told module authors exactly what they needed to provide if they wanted to save a new term or vocabulary, but the new docs aren't quite there yet.

2. - *   The vocabulary object with all of its metadata, if exists, FALSE otherwise.
   + * @return TaxonomyVocabulary|false
   + *   The taxonomy vocabulary entity, if exists, FALSE otherwise.
     *   Results are statically cached.
   

   Minor issue here with the line not wrapping at 80 characters.
   

3. @@ -526,7 +528,6 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
    
        // Load and save a term, confirming that parents are still set.
        $term = taxonomy_term_load($term2->tid);
   -    taxonomy_term_save($term);
        $parents = taxonomy_get_parents($term2->tid);
        $this->assertTrue(isset($parents[$term1->tid]), t('Parent found correctly.'));
   

   I don't understand why this line is being removed? Based on the code comment, it sounds like it's integral to what the test is trying to assert... Anyway, at a minimum the code comment is no longer consistent with what the code is actually doing :)

1. As I understand it are the _save() functions just a temporary BC wrapper (that has already been removed in the user patch because the function would have to be changed anyway). The optional original property should IMHO be documented directly on the Entity class because it's not taxonomy specific. Not sure where information on how to create entities from scratch should live, but I think it's rather pointless to add it to a function that we want to remove anyway.

3. No idea either, pretty sure that line needs to stay.

Rebased and re-rolled #47 in http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo... against latest HEAD.

Does *not* incorporate any feedback since #47.

There is now also a langcode both on vocabularies and terms. I did not find those coved in the patch on a quick look.

• fixed type hinting on forum_taxonomy_term_delete()
• IMHO taxonomy_term_save() is a legacy function and it should be marked as deprecated. So there is no point in adding documentation there. (#51)
• fixed line wrapping in doc blocks on taxonomy_vocabulary_machine_name_load() and taxonomy_vocabulary_load() (#51)
• fixed the missing taxonomy_term_save() in the test (#51)

Interdiff is against #54. Does no incorporate feedback since #52.

Created a branch in sun's platform sandbox and pushed my patch there: http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo...

I've dissected and merged in all changes by @amateescu and @klausi.

See curent branch for all changes.

(@klausi: Merging in your changes was a bit harder, as your branch wasn't based on platform-taxonomy. Therefore, I've removed that branch. Please create a new branch based on the main one if you'd like to work on this further.)

Looks like only #53 still needs to be incorporated.

Sorry.

IMHO taxonomy_term_save() is a legacy function and it should be marked as deprecated. So there is no point in adding documentation there. (#51)

In #51 I proposed not having the documentation remain on taxonomy_term_save() itself, but rather moving it elsewhere (probably to the class). However, the current patch still seems to just delete it without adding it back anywhere else.

Reviewed fago's changes from #63 and they all look very good.

Re: #51 1)

• The list of optional properties isn't as complete.
• It's missing an indication of which properties are optional and which are required when creating a new object.

Fixed in the attached patch.

• It doesn't explain that you use entity_create() to do so.

I think we can leave this for a follow-up task. We have three more entity conversions that are waiting for this one to land :)

Okay, time to get this in, and continue with entity conversions.
The current patch looks good to me, and I see that all feedback so far has been addressed.

+++ b/core/modules/entity/entity.class.incundefined
@@ -179,9 +205,14 @@ class Entity implements EntityInterface {
+  public function enforceIsNew($value = TRUE) {
+    $this->enforceIsNew = $value;

enforceIsNew is hard to say, what about forceNew()? But that doesn't have the strong link to isNew so not sure... either way can be a follow-up if at all.

+++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -298,6 +298,10 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+    // Re-register autoload functions.
+    spl_autoload_register('drupal_autoload_class');
+    spl_autoload_register('drupal_autoload_interface');

Why do we need to do this?

Leaving at RTBC, would be good to get feedback on those, but I'm happy committing this later on anyway.

related #1495024: Convert the entity system to PSR-0

Sounds good.

Re #68: The database-backed class loader doesn't play nice with Unit tests and, apparently, some functional tests. In order to keep it from exploding due to a missing DB etc. you need to disable it in setUp and re-enable it in shutDown. Arguably we should just push that into the Unit test case base class, but I haven't gotten around to doing so.

I haven't reviewed its usage in this patch to say why it's being done here, since it's a WebTestCase, but I presume it's the same reasoning.

Thanks folks. We've been badly over thresholds the past week, but currently if you take the RTBC patches against 7.x from the threshold count it's just on the line, so since this gets us towards closing #1368394: Convert all core entities to classed objects which is also over thresholds I've gone ahead and committed/pushed it to 8.x.

This needs a change notification, but we probably want to have just one for the meta issue, going to mark this as needing one but hopefully we can just add a short note to an overview of the changes.

I think we alrdy have one: http://drupal.org/node/1400186

That looks good to me, marking fixed. Thanks!

Regression: #1550454: Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices

Sorry; I started to reopen this but then decided it was better to file a followup (since we are several entity conversions past reverting this), but then didn't set the fields back. :P

Updated issue summary.