Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Spleshka’s picture

Hi,

I think that you are right, and support taxonomy and entities would be great. But currently I am not sure if I have enough time to implement this. I would appreciate any help in the development, if any.

arrubiu’s picture

I'll take a look to your implementation of user and files expiration, maybe it could be easy to implements files :)

Spleshka’s picture

Yes, it is really easy to do this. In general you may take a look on users' expiration and copy most of code for taxonomy.

capellic’s picture

But the home page says that Taxonomy is supported. Confused.

Current (7.x-2.x) branch has configurable expiration rules out of the box for:

Nodes
Comments
Users
Taxonomy
Menus
Files
VotingAPI votes

Spleshka’s picture

That means that we have a support of taxonomy references. But you are right, that may be confusing.

mukhsim’s picture

Patch attached, please test.

medienverbinder’s picture

Hi!

I think taxonomy is basically supported. If a node has been modified, the associated taxonomy terms were deleted from the cache. However when a taxonomy term is changed, there was no response to this change.

With the patch (#6) so far everything is running correctly.

Server:
Varnish 3 proxy cache server with purge support

Drupal Modules:
Varnish
Purge
Expire

After applying the patch, I was able to consider the changes with the expire and purge module and delete taxonomy terms from cache with the corresponding ban command:

Example ban command after saving a taxonomy term (tid 34):
varnish Successfully ran command: ban req.http.host ~ [CACHESERVERURL] && req.url ~ "^/$|^/home$|^/taxonomy/term/34$|^/footerm$"

Thank you!

quotesBro’s picture

Version: 7.x-2.0-rc2 » 7.x-2.x-dev
Status: Active » Needs review
FileSize
15.21 KB

Thanks for #6, it works for me.
Here is a patch #6 with 2 fixes:

+/**
+ * Settings form for a node type.
+ */

->

+/**
+ * Settings form for a taxonomy vocabulary.
+ */

and
+ '#default_value' => variable_get('expire_node_override_defaults_' . $vocabulary),
->
+ '#default_value' => variable_get('expire_taxonomy_term_override_defaults_' . $vocabulary),

dysrama’s picture

+1 for patch #6, works as intended.
Patch #8 doesn't work though, since it fails to add the expire.taxonomy_term.inc file

joelpittet’s picture

@dberror an interdiff would be nicer;) @see https://drupal.org/documentation/git/interdiff

There must have been an issue because the things you said changed made the patch ~4kb smaller. And by a quick interdiff it looks like the new file was forgotten to be added (That is super common I've seen).

joelpittet’s picture

Here is #8 again because they were nice catches in there, with an interdiff between #6 and #8.

joelpittet’s picture

Ha, and I did that backwards... whoops.

joelpittet’s picture

Status: Needs review » Needs work

Here's a review, lots of nitpicks for coding standards:

  1. +++ b/expire.admin.inc
    @@ -192,6 +192,111 @@ function expire_admin_settings_form() {
    +  // TAXONOMY TERM SETTINGS.
    

    Shouldn't be all caps.

  2. +++ b/expire.admin.inc
    @@ -192,6 +192,111 @@ function expire_admin_settings_form() {
    +  if (module_exists('taxonomy')) {
    

    Is this check needed?

  3. +++ b/expire.admin.inc
    @@ -764,3 +869,81 @@ function expire_node_settings_form(&$form) {
    +
    +
    

    Extra line break added.

  4. +++ b/expire.admin.inc
    @@ -764,3 +869,81 @@ function expire_node_settings_form(&$form) {
    +  $form['expire']['taxonomy_term_actions']['expire_taxonomy_term_actions']['#default_value'] =
    +    variable_get('expire_taxonomy_term_actions_' . $vocabulary, array());
    ...
    +  $form['expire']['taxonomy_term_expires']['expire_taxonomy_term_front_page']['#default_value'] =
    +    variable_get('expire_taxonomy_term_front_page_' . $vocabulary, EXPIRE_TAXONOMY_TERM_FRONT_PAGE);
    ...
    +  $form['expire']['taxonomy_term_expires']['expire_taxonomy_term_taxonomy_term_page']['#default_value'] =
    +    variable_get('expire_taxonomy_term_taxonomy_term_page_' . $vocabulary, EXPIRE_TAXONOMY_TERM_TAXONOMY_TERM_PAGE);
    

    Probably shouldn't break this onto a new line for coding standards.

  5. +++ b/expire.admin.inc
    @@ -764,3 +869,81 @@ function expire_node_settings_form(&$form) {
    +  if (module_exists('taxonomy')) {
    

    Again this is checking for taxonomy, maybe it shouldn't be as this whole thing depends on taxonomy.

  6. +++ b/expire.admin.inc
    @@ -764,3 +869,81 @@ function expire_node_settings_form(&$form) {
    +    if (module_exists('field_collection')) {
    

    Is this field_collection module check nessassary or can we punt that to a hook to alter if a 3rd party wants to extend expire?

  7. +++ b/expire.drush.inc
    @@ -56,6 +56,21 @@ function expire_drush_command() {
    +    'callback' => 'drush_expire_term'
    

    Needs a comma at the end.

  8. +++ b/expire.drush.inc
    @@ -156,10 +178,15 @@ function _drush_expire_entity($entity_type) {
    +  print 'hadlner: ' . print_r($handler, 1);
    +  print print_r($entity_type, 1);
    +  print print_r($ids, 1);
    

    Should these be drush_print() and drush_print_r() statements? Also "handler" has a typo.

  9. +++ b/expire.drush.inc
    @@ -156,10 +178,15 @@ function _drush_expire_entity($entity_type) {
    +//  print print_r($entities, 1);
    

    Debug?

Leon Kessler’s picture

Would love to see this in.

I've gone through the patch and comments from #13.
Uploaded a new patch + interdiff.

Will explain changes in a follow up comment.

Leon Kessler’s picture

Minor adjustment.
Here's new patch and interdiff from #12

Comments to follow...

Leon Kessler’s picture

Status: Needs work » Needs review

Okay so...

1)
Removed module_exists('taxonomy') checks (I think these have just been copied over from the node settings).
We could wrap the whole thing in a module_exists check, not sure if it would be worth it.

2)

+++ b/expire.admin.inc
@@ -245,16 +245,7 @@
-    $form['tabs']['taxonomy_term']['expire']['expire_taxonomy_term_term_pages'] = array(
-      '#type' => 'checkbox',
-      '#title' => t('Taxonomy term pages'),
-      '#description' => t('Expire urls of terms that are selected in the expiring taxonomy term.'),
-      '#default_value' => variable_get('expire_taxonomy_term_term_pages', EXPIRE_TAXONOMY_TERM_TERM_PAGES),
-    );
-  }

Removed this entire setting, as it didn't really make sense on the config page.
You had:
"Taxonomy term page."
and then
"Taxonomy term pages"
Again this has been copied from the node settings (where you can expire a nodes term pages). Instead I've moved everything to "Taxonomy term reference pages". This now takes care of the lot (taxonomy_term_reference, node_reference, user_reference and entityreference).

3)

+++ b/expire.admin.inc
@@ -245,16 +245,7 @@
-  if (module_exists('taxonomy_term_reference') || module_exists('user_reference') || module_exists('entityreference')) {
+  if (module_exists('node_reference') || module_exists('user_reference') || module_exists('entityreference')) {

@@ -923,12 +913,7 @@
-  if (module_exists('taxonomy_term_reference') || module_exists('user_reference') || module_exists('entityreference')) {
+  if (module_exists('node_reference') || module_exists('user_reference') || module_exists('entityreference')) {

taxonomy_term_reference is not a module, it's a field type contained within the taxonomy module. We still need to check for node_reference module.

4)

+++ b/expire.drush.inc
@@ -68,7 +68,7 @@
@@ -180,13 +180,8 @@

@@ -180,13 +180,8 @@
   $handler = _expire_get_expiration_handler($entity_type);
-
-  print 'hadlner: ' . print_r($handler, 1);
-  print print_r($entity_type, 1);
-  print print_r($ids, 1);
   $entities = entity_load($entity_type, $ids);
   if (is_object($handler) && !empty($entities)) {
     foreach ($entities as $entity) {
       $handler->expire($entity, 0, $skip_action_check = TRUE);
     }
   }
-//  print print_r($entities, 1);

Removed all debug code from _drush_expire_entity

5)

Have cleaned up Drupal coding standards, including some of the comments in includes/expire.taxonomy_term.inc (which went over 85 characters).

6)

+++ b/expire.admin.inc
@@ -192,6 +192,111 @@ function expire_admin_settings_form() {
+  // TAXONOMY TERM SETTINGS

I've left this in, because it's how all the other sections are commented, and doesn't strictly break coding standards.

7)
Have also left in the line breaks in expire_taxonomy_term_settings_form(). Again this mimics how the rest of the module is done, and also doesn't break actually coding standards.

joelpittet’s picture

This looks much better @leon.nk thanks for the cleanup and explaining your changes in detail.

I'm going to give this a whirl and see how it goes.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Seems to work very well. Thank you @leon.nk!

  • Spleshka committed 0bacb20 on 7.x-2.x authored by leon.nk
    Issue #2201913 by joelpittet, leon.nk, mukhsim, dberror, Spleshka: Add...
Spleshka’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your patches and reviews guys! Added few changes and commited to 7.x-2.x.

Status: Fixed » Closed (fixed)

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

sam.spinoy@gmail.com’s picture

Status: Closed (fixed) » Needs work

The "Override default settings for this vocabulary" option does not seem to work? The options are present on the admin/structure/taxonomy//edit page but they don't get saved.

smitty’s picture

I can confirm that the overridings made for the different Vocabularys are not saved.

I assume, that's because the functions "expire_taxonomy_vocabulary_update" "expire_taxonomy_vocabulary_insert" which could be called by taxonomy_vocabulary_save (taxonomy module) are missing. The same with "expire_taxonomy_vocabulary_delete". (at least the Metatag Module defines such functions to get the changes made to the taxonomy form saved).

earelin’s picture

The override of vocabulary does not work. I agree with comments from #23. I have created a patch that fix this problem using hook_taxonomy_vocabulary_update|insert. I had to change the form to be a tree for easier process of the data on the vocabulary array.

devad’s picture

Patch #24 works fine if vocabulary update action is launched from standard vocabulary edit tab:

http://mysite.com/admin/structure/taxonomy/vocab_name/edit

However, if vocabulary udate action is launched from vocabulary term list:

http://mysite.com/admin/structure/taxonomy/vocab_name

it is loosing some already saved expire variables and giving following notices:

Notice: Undefined property: stdClass::$expire in expire_taxonomy_vocabulary_update() (line 166 of /home/mysite/public_html/sites/all/modules/expire/expire.module).
Notice: Undefined property: stdClass::$expire in expire_taxonomy_vocabulary_update() (line 168 of /home/mysite/public_html/sites/all/modules/expire/expire.module).
Notice: Undefined property: stdClass::$expire in expire_taxonomy_vocabulary_update() (line 169 of /home/mysite/public_html/sites/all/modules/expire/expire.module).
Warning: Invalid argument supplied for foreach() in expire_taxonomy_vocabulary_update() (line 169 of /home/mysite/public_html/sites/all/modules/expire/expire.module).

I have wrapped problematic piece of code with "if (isset($vocabulary->expire))" to prevent notices and problems and it worked.

devad’s picture

Status: Needs work » Needs review
FileSize
7.29 KB

New patch.

devad’s picture

FileSize
7.13 KB

Tried to fix line endings... I hope #27 is ok.

joelpittet’s picture

This should likely be in a new issue and made reference here. Usually we don't re-open issues unless it needs to be reverted.

smitty’s picture

The last patch works like a charm!

Please commit it.

smitty’s picture

Unfortunately, it is not possible to expire the nodes which contain a taxonomy term, if the taxonomy term is changed. It would be great if this feature could be implemented too.

I created an own issue for this: https://www.drupal.org/node/2695017