Forum vocabulary alterations possibly obsolete -- possible to delete forum vocab

Seems reasonable and could be easily fixed. But first need UX attention

There is an existing issue dealing with this. It's a bug in the form alter code, the form is has. changed so the vocabulary options are the same.
Re forum node form, that's from node_forms. The dup issue is #10566: Forum can not appear in multiple containers

This issues are different. Pointed issue should fix visibility of forum in multiple containers (multi-parented)

But this issue about all forum_form_alter() so let's not mix them.

Just a tests

A bit smarter tests, now we can be sure that fieldset has relations name in form definition

Introduced hook_form_alter() split into forum_form_taxonomy_form_vocabulary_alter() and forum_form_taxonomy_form_term_alter() for performance reasons.

Not sure this split could be allowed for D7...

Updated issue summary for D8. patch with better comments

Finally added @see to hooks doc block

I don't think we should prevent ability to remove forum's field from forum node type

D7 patch to test

D7 has no TAXONOMY_HIERARCHY_* contants

Back to 8.x

This could cause deletion of vocabulary

#10 - patch for D8
#14 - D7

#10: 787652-forum-form_alter-split.patch queued for re-testing.

+++ b/core/modules/forum/forum.module
@@ -575,24 +575,34 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+  if (isset($form['vid']['#value']) && $form['vid']['#value'] == $vid) {

I think we should add a test that verifies this field exists as well, so if it is removed we don't end up with broken logic again.

Powered by Dreditor.

This one without patch, to verify tests fail.
SHOULD FAIL.

This one adds test for vid field as per 19

helps if I load the include...

+++ b/core/modules/forum/forum.moduleundefined
@@ -575,24 +575,34 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+ * @see taxonomy_form_vocabulary()

+++ b/core/modules/forum/forum.testundefined
@@ -246,6 +246,34 @@ class ForumTestCase extends DrupalWebTestCase {
+    // Test vid value field exists
+    $vocabulary = taxonomy_vocabulary_machine_name_load('forum');
+    module_load_include('inc', 'taxonomy', 'taxonomy.admin');
+    $form = drupal_get_form('taxonomy_form_vocabulary', $vocabulary);
+    $this->assertNotNull($form['vid'], t('Vocabulary Id field found'));
+    $this->assertNotNull($form['vid']['#value'], t('Vocabulary Id found'));

I think this is a over-engineering because if we are adding/editing vocabulary then vID field could be optional. For this purpose taxonomy_form_vocabulary() mentioned in @see
so if core would going to tremove primary key for vocabulary then we find this place

EDIT inline comments should end-up with comma

Hey, the way I'm invoking it, vid should never be null (I'm passing $vocabulary).
Patch attached fixes inline comments and also changes assert from notNull to true.

Wrong machine name

+++ b/core/modules/forum/forum.moduleundefined
@@ -575,24 +575,34 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
- * Implements hook_form_alter().
+ * Implements hook_form_FORM_ID_alter().
+ *
+ * @see taxonomy_form_vocabulary()

This should be hook_form_FORM_ID_alter() for xyz(). See: http://drupal.org/node/1354#hookimpl.

+++ b/core/modules/forum/forum.moduleundefined
@@ -575,24 +575,34 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+    // Forum's vocabulary always has single hierarchy. Forums and containers has
+    // only one parent or no parent for root items. By default this value is 0.

Forums and containers have?

+++ b/core/modules/forum/forum.moduleundefined
@@ -575,24 +575,34 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+ * Implements hook_form_FORM_ID_alter().
+ *
+ * @see taxonomy_form_term()

Another hook_form_FORM_ID_alter() implementation, as above.

+++ b/core/modules/forum/forum.testundefined
@@ -246,6 +246,34 @@ class ForumTestCase extends DrupalWebTestCase {
+    $this->assertFieldByName('op', t('Save'), t('Save button found.'));

Messages that are only used in tests shouldn't be translated. That means: Yes, do use t('Save'), but don't use t() for 'Save button found.'. The same thing goes for the other assertions.

Let's get one more opinions

+++ b/core/modules/forum/forum.moduleundefined
@@ -575,24 +575,34 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+  if (isset($form['vid']['#value']) && $form['vid']['#value'] == $vid) {

suppose we should remove isset() so if once 'vid' would be removed this alter fails

+++ b/core/modules/forum/forum.testundefined
@@ -246,6 +246,34 @@ class ForumTestCase extends DrupalWebTestCase {
+    // Test vid value field exists.
+    $vocabulary = taxonomy_vocabulary_machine_name_load('forums');
+    module_load_include('inc', 'taxonomy', 'taxonomy.admin');
+    $form = drupal_get_form('taxonomy_form_vocabulary', $vocabulary);
+    $this->assertTrue(isset($form['vid']), t('Vocabulary Id field found'));
+    $this->assertTrue(!empty($form['vid']['#value']), t('Vocabulary Id found'));
+

this make this part useless

OK, anypost and I spoke on IRC about this and I realise the test I proposed at #19 is superfluous - other tests will fail if this field is removed upstream.

So this is the patch at #14 rerolled to take into account comments from #30 plus some other comment amendments.

Excellent :)
One minor point:

+++ b/core/modules/forum/forum.moduleundefined
@@ -575,29 +575,40 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+ *
+ * @see taxonomy_form_vocabulary()

(And sorry, I could have made it clearer.) The @see is superflous now.

+++ b/core/modules/forum/forum.moduleundefined
@@ -575,29 +575,40 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+ *
+ * @see taxonomy_form_term()

This one, too.

All good

+1 to RTBC, @larowlan thanx for fixing comments and grammar

+++ b/core/modules/forum/forum.moduleundefined
@@ -575,29 +575,36 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+}
+/**

Ugg. Another tiny thing I missed. An empty line is supposed to be here. Leaving needs review for a real review.

Nice catch

This looks good to me, so I'm marking RTBC. I'll commit in a couple of days if no objections.

Committed/pushed. Moving to 7.x for backport.

This was reverted.

#37: 787652-forum-form_alter-split_37.patch queued for re-testing.

Testing profile used for test now has no Tags vocabulary

Patch seems to be mangled, can't view it, can you please reattach?
Weird that tests passed!

Same patch

This looks very strange, I've seen this patch after upload and it was ok. Probably there's some troubles with d.o infra

This one worked fine.

Oof. What a mess. :) Good catch!

Committed and pushed to 8.x. This needs a quick re-roll for D7, for the TAXONOMY_HIERARCHY_* stuff if nothing else.

patches for D7

787652-forum-form_alter-FULL-split-48.patch splits hook_form_alter to 3 hooks

787652-forum-form_alter-split-48.patch just fixes wrong keys

for performance reasons it's recommended to commit FULL patch because it's just a hook split

Let's leave it for @webchick to decide

EDIT: is D7 uses testing profile? Looks like Tags vocabulary is still created and used in tests.

#48: 787652-forum-form_alter-FULL-split-48.patch queued for re-testing.

@larowlan please review this

+1 for the FULL version but either are fine.

I think it's fine to rename hook implementations. Committed and pushed the FULL patch 7.x for parity with 8.x. Thanks!