Date coder for pretty paths

Hi,

Thanks a lot for your contribution! I'm on my mobile phone right now so can't really do an in-depth review atm.

The first question that I have though is, why would we need another taxonomy no-id coder? That should already be covered by the pathauto coder.

Regards

We using this modules in production so glad to see this bunch of fixes shipped with the module

i'm not yet convinced of introducing the "taxonomy no id" as it does almost the same as the pathauto coder.

maybe we should merge those into a single one with a setting on which strategy to choose from: a) just use the term name, b) use pathpauto if available.

note that there is also some work going on in #1981578: Allow to remove "Taxonomy Vocabulary" from urls.

besides my conceptual concerns, here's a quick code review:

1. +++ b/facetapi_pretty_paths.module
   @@ -47,60 +47,129 @@ function facetapi_pretty_paths_form_facetapi_facet_display_form_alter(&$form, &$
   +    // Load all coders for <strong>oprion</strong> list.
   

   typo

2. +++ b/facetapi_pretty_paths.module
   @@ -47,60 +47,129 @@ function facetapi_pretty_paths_form_facetapi_facet_display_form_alter(&$form, &$
   +    $default_<strong>sected</strong>_coder = 'default';
   

   typo

3. +++ b/plugins/coders/facetapi_pretty_paths_coder_taxonomy_no_id.inc
   @@ -0,0 +1,73 @@
   +    if (count($exploded) == 1) {
   +
   +      $facet_settings = $args['adapter']->getFacetSettingsGlobal($args['facet']);
   +      $vocabulary = !empty($facet_settings->settings['pretty_paths_taxonomy_no_id_vocabulary']) ? $facet_settings->settings['pretty_paths_taxonomy_no_id_vocabulary'] : NULL;
   +
   +      $terms = taxonomy_get_term_by_name($exploded[0], $vocabulary);
   +      $term = reset($terms);
   +      if (is_object($term)) {
   +        $args['value'] = $term->tid;
   +      }
   +      else {
   +        $args['value'] = '';
   +      }
   +    }
   +    else if (!is_numeric($exploded[count($exploded) - 1])) {
   +      $facet_settings = $args['adapter']->getFacetSettingsGlobal($args['facet']);
   +      $vocabulary = !empty($facet_settings->settings['pretty_paths_taxonomy_no_id_vocabulary']) ? $facet_settings->settings['pretty_paths_taxonomy_no_id_vocabulary'] : NULL;
   +      $terms = taxonomy_get_term_by_name($args['value'], $vocabulary);
   +      $term = reset($terms);
   +      if (is_object($term)) {
   +        $args['value'] = $term->tid;
   +      }
   +      else {
   +        $args['value'] = '';
   +      }
   +    }
   

   quite some code duplication across the two if clauses

+++ b/plugins/coders/facetapi_pretty_paths_coder_dates.inc
@@ -0,0 +1,84 @@
+    //decode to solr dates diapason

please respect coding standards:
// Decode to solr dates diapason.

also i have to admit that i haven't heard of diapason before and i a quick google on "solr date diapason" didn't lead to any meaningful results. does using something like "value" instead work?

1. +++ b/plugins/coders/facetapi_pretty_paths_coder_dates.inc
   @@ -0,0 +1,84 @@
   +    }
   ...
   +    if (count($all_dates) == 2) {
   +      $first_date_timestamp = strtotime($all_dates[0]);
   +      $first_date = date($solr_date_format_for_path, $first_date_timestamp);
   +
   +      $second_date_timestamp = strtotime($all_dates[1]);
   +      $second_date = date($solr_date_format_for_path, $second_date_timestamp);
   +    }
   +    else {
   +      $first_date_timestamp = strtotime($all_dates[0]);
   +      $first_date = date($solr_date_format_for_path, $first_date_timestamp);
   +
   +      $second_date_offset = '+1 ' . strtolower($facet_settings->settings['date_granularity']);
   +      $second_date = date($solr_date_format_for_path, strtotime($all_dates[0] . $second_date_offset));
   

   code duplication across if statements

2. +++ b/plugins/coders/facetapi_pretty_paths_coder_dates.inc
   @@ -0,0 +1,84 @@
   +
   +
   

   unnecessary empty line

Patch addresses 2 issues:

- terms are displayed in urls with capitalization retained.
- pretty_paths_taxonomy_coder_strategy is not always set, so check for it.

I have the impression the code is still doing things to terms when it is not configured to do so?

My interdiff kinda failed (dunno why), but here's the relevant part:

diff --git a/plugins/coders/facetapi_pretty_paths_coder_taxonomy.inc b/plugins/coders/facetapi_pretty_paths_coder_taxonomy.inc
index de63a17..ac46b78 100644
--- a/plugins/coders/facetapi_pretty_paths_coder_taxonomy.inc
+++ b/plugins/coders/facetapi_pretty_paths_coder_taxonomy.inc
@@ -17,7 +17,8 @@ class FacetApiPrettyPathsCoderTaxonomy extends FacetApiPrettyPathsCoderDefault {
    */
   public function encodePathSegment(array $args) {
     $facet_settings = $args['adapter']->getFacetSettingsGlobal($args['facet']);
-    if ($facet_settings->settings['pretty_paths_taxonomy_coder_strategy'] == 'taxonomy_coder') {
+
+    if (isset($facet_settings->settings['pretty_paths_taxonomy_coder_strategy']) && $facet_settings->settings['pretty_paths_taxonomy_coder_strategy'] == 'taxonomy_coder') {
       if ($term = taxonomy_term_load($args['segment']['value'])) {
         $args['segment']['value'] = $this->prettyPath($term->name) . '-' . $term->tid;
       }
@@ -30,10 +31,10 @@ class FacetApiPrettyPathsCoderTaxonomy extends FacetApiPrettyPathsCoderDefault {
 
         $terms = taxonomy_get_term_by_name($term->name, $vocabulary);
         if (count($terms) == 1) {
-          $args['segment']['value'] = $term->name;
+          $args['segment']['value'] = $this->prettyPath($term->name);
         }
         else {
-          $args['segment']['value'] = $term->name . '-' . $term->tid;
+          $args['segment']['value'] = $this->prettyPath($term->name) . '-' . $term->tid;
         }
       }
     }

you have my vote for the taxonomy functionality as provided by the patch.

the provided patches still include the "no id" coder, which i'd like to be solved by #1981578: Allow to remove "Taxonomy Vocabulary" from urls

i'd suggest turning this one into a stand-alone "dates coder" patch.

and tests

andypost, was that comment intentional? not sure about the tag :)

@dasjo I'm trying to follow facetapi 8 so just marked to check later. I found that few sites still using the patch #7, so maybe d8 version would be a different

hi andypost & uzlov,

thanks for getting this going again, your contribution is very much appreciated :)

i only have a few small remarks but it looks like we can get this in soon:

1. +++ b/facetapi_pretty_paths.module
   @@ -313,7 +351,8 @@ function facetapi_pretty_paths_facetapi_facet_info_alter(array &$facet_info, arr
   +    ¶
   

   please make sure that indentation (spaces) are correct

2. +++ b/plugins/coders/facetapi_pretty_paths_coder_dates.inc
   @@ -0,0 +1,86 @@
   +//      $value = date($date_format_for_path, strtotime($first_date)) . '_to_' . date($date_format_for_path, strtotime($second_date));
   

   can we remove that one?

3. +++ b/plugins/coders/facetapi_pretty_paths_coder_dates.inc
   @@ -0,0 +1,86 @@
   +    $delimiter = t('@date1_to_@date2', array('@date1' => '', '@date2' => ''));
   +    $all_dates = explode($delimiter, $args['value']);
   

   could you add a comment, so that $arg['value] is expected to be like TIMESTAMP1_to_TIMESTAMP2

4. +++ b/plugins/coders/facetapi_pretty_paths_coder_dates.inc
   @@ -0,0 +1,86 @@
   +    if ($first_date != '' && $second_date != '') {
   

   Is there a specific reason why only the date part without times is respected? Probably because you want to to be compact. We could add an optional setting so that aliases contain the time part as well?

thanks, looks good haven't tested it yet. adapting issue title