CommentFileSizeAuthor
#49 interdiff-38-49.txt8.53 KBRoSk0
#49 features_extra-add-date-format-support-1279928-49.patch11.93 KBRoSk0
PASSED: [[SimpleTest]]: [MySQL] 81 pass(es). View
#38 features_extra-add-date-format-support-1279928-38.patch11.86 KByingtho
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features_extra-add-date-format-support-1279928-38_0.patch. Unable to apply patch. See the log in the details link for more information. View
#15 features-date-1279928-15.patch7.33 KBderhasi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date-1279928-15.patch. Unable to apply patch. See the log in the details link for more information. View
#10 features-date_formats-1279928-10.patch7.39 KBderhasi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date_formats-1279928-10.patch. Unable to apply patch. See the log in the details link for more information. View
#6 0001-Support-for-cores-date-formats.patch7.57 KBmrfelton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0001-Support-for-cores-date-formats.patch. Unable to apply patch. See the log in the details link for more information. View
#5 features-date_formats-1279928-5.patch7.12 KBderhasi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date_formats-1279928-5.patch. Unable to apply patch. See the log in the details link for more information. View
#3 features-date_formats-1279928-3.patch7.08 KBderhasi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date_formats-1279928-3.patch. Unable to apply patch. See the log in the details link for more information. View
#2 features-date_formats-1279928-2.patch4.95 KBderhasi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date_formats-1279928-2.patch. Unable to apply patch. See the log in the details link for more information. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

perarnet’s picture

I tried your patch from http://drupal.org/node/1083394#comment-4857390, but can't see date_formats showing up.

derhasi’s picture

Issue tags: +date_format
FileSize
4.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date_formats-1279928-2.patch. Unable to apply patch. See the log in the details link for more information. View

There is a reworked patch for this issue. (Forgot to include the system module)

derhasi’s picture

FileSize
7.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date_formats-1279928-3.patch. Unable to apply patch. See the log in the details link for more information. View

And there it is with support for date format types.

mrfelton’s picture

Status: Needs review » Needs work

This causes a fatal error if you dont have the locale module enabled.:

Fatal error: Call to undefined function locale_language_list() in /Users/tom/workspace/sac2/sites/all/modules/contrib/features/includes/features.system.inc on line 36

derhasi’s picture

Status: Needs work » Needs review
FileSize
7.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date_formats-1279928-5.patch. Unable to apply patch. See the log in the details link for more information. View

Oh, sure fixed that.

mrfelton’s picture

FileSize
7.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0001-Support-for-cores-date-formats.patch. Unable to apply patch. See the log in the details link for more information. View

Thats better, although it doesn't apply cleanly, so can't use it with Drush make. Here is one that does.

mrfelton’s picture

Confirm that the patch worked. When you have strongarm enabled, you also get the option to export a variable fo r each date format. Are these still needed with using the above? Do you know how the system variables relate to the other database settings that are being managed by this patch?

derhasi’s picture

The variables are for the default format of the exported date format type (see hook_date_format_types). In the current patch they are set in date_formats_features_rebuild(), as this is an obligatory step for enabling/setting the date format correctly.

So dependency on strongarm was not good. But we could add the variables to the pipe, so they get exported automatically (in most cases) to the relevant feature.

edit: mrfelton, please look for the patch naming convention... so a patch can be easily related to the issue number ;)

mpotter’s picture

I'd like to see a bit more work before committing this to deal with the variables issue. I've got sites exporting custom date types via strongarm which works fine. What am I gaining by having this in Features instead of just using Strongarm? What happens on my sites that already export the variables...will I need to rebuild those features?

derhasi’s picture

FileSize
7.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date_formats-1279928-10.patch. Unable to apply patch. See the log in the details link for more information. View

Attached is a new patch with the pipe-part for strongarm applied to the current dev.

mpotter, to your annotations: using features for date_formats covers also locale date formats. If you haven't strongarm enabled, you still could export the date formats.

If it's ok to have a dependency for that core component, I'd propose to rename the "Date formats" component to "Locale date formats" and only deal with locale date formats for that component.

How about additional pipes?
* locale date formats => language
* date format type => strongarm:date_format

derhasi’s picture

mpotter, do u have any opinion on the road to go?

mpotter’s picture

sorry, been busy at DrupalCon. I'll try to evaluate this next week. I'd like to feature-freeze features 1.x so we can get 1.0 released soon, but if this works with no negative impact on existing sites then I will see about getting it in.

Getting some other people to help review this also would be a big help.

hefox’s picture

Status: Needs review » Needs work
+++ b/includes/features.system.inc
@@ -0,0 +1,222 @@
+    'date_format_types' => $type_pipes,
+    'variable' => $strongarm_pipes,
+  );
+  return $pipe;

Why the separate variables instead of $pipe['date_format_types'] .. ? Looks a bit unnecessaryly messy.

Adding information for another module (strongarm) feels a bit weird to me.

+++ b/includes/features.system.inc
@@ -0,0 +1,222 @@
+  // The filter_default_formats() hook integration is provided by the

Filter_default_formats?

+++ b/includes/features.system.inc
@@ -0,0 +1,222 @@
+        variable_set('date_format_'. $spec['type'], $spec['format']);

This whole variable_set vs strongarm thing is weird. Really not comfortable for it.

+++ b/includes/features.system.inc
@@ -0,0 +1,222 @@
+
+/**
+ * Implements hook_features_rebuild().
+ *
+ * @see hook_date_format_types()
+ */
+function date_format_types_features_rebuild($module) {
+  // Anything should be managed by hook_date_format_types().
+}
+

Don't see any reason for this; just don't implement the hook. rebuild is optional. Can people override default date formats? If so, do need a _revert but not a rebuild.

+++ b/includes/features.system.inc
@@ -0,0 +1,222 @@
+    $format = unserialize(db_query('SELECT value FROM {variable} WHERE name = :name', array(':name' => "date_format_". $type))->fetchField());

Not checking if that actually returns anything than running unserialize? ow ow.

+++ b/includes/features.system.inc
@@ -0,0 +1,222 @@
\ No newline at end of file

New line

I sorta think this should go into something that depends on strongarm, though not sure what that'd be (a sub module of features? a submodule of strongarm? a separate project?)

derhasi’s picture

Ok, I will rework that soon to the " locale date formats" approach. So we have no fuzzy dependencies.

derhasi’s picture

Status: Needs work » Needs review
FileSize
7.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-date-1279928-15.patch. Unable to apply patch. See the log in the details link for more information. View

And finally there's the reworked patch for that issue.

* date format types are implemented via core hook.
* Strongarm is "piped" for default formats of a type.
* locale date formats are exportable via component: pipes its date format type.

@hefox, noticed your comments and put it into code.

IMHO using the weak dependency on strongarm is the best way, as strongarm is a common contrib.

mrfelton’s picture

Nice. Works well. Although applying the patch gives a whitespace warning:

features-date-1279928-15.patch:154: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
alex.designworks’s picture

Tried patch in #15 against 7.x-1.0-rc2 - new date types and format does not appear.
Workflow:
1. Patched Features module and copied to Site1 and Site2
2. Created new date type
3. Created new date format
4. Assigned date format to date type

5. Created Feature, containing only date type on Site1 and copied it to Site2
6. Enabled feature on Site2
7. Cleared all cache

8. Neither date type nor date format appeared under admin/config/regional/date-time.

Feature code below:

/**
 * @file
 * cic_date.features.inc
 */

/**
 * Implements hook_ctools_plugin_api().
 */
function cic_date_ctools_plugin_api() {
  list($module, $api) = func_get_args();
  if ($module == "strongarm" && $api == "strongarm") {
    return array("version" => "1");
  }
} 
/**
 * @file
 * cic_date.strongarm.inc
 */

/**
 * Implements hook_strongarm().
 */
function cic_date_strongarm() {
  $export = array();

  $strongarm = new stdClass();
  $strongarm->disabled = FALSE; /* Edit this to true to make a default strongarm disabled initially */
  $strongarm->api_version = 1;
  $strongarm->name = 'date_format_short_string';
  $strongarm->value = 'F j, Y';
  $export['date_format_short_string'] = $strongarm;

  return $export;
}

@derhasi
Are there any actions to be done to make it work?

mrfelton’s picture

@alex.designworks Are you sure you included the date type component in your feature? Doing so should result in an implementation of hook_date_format_types() in your feature.

alex.designworks’s picture

Are you sure you included the date type component in your feature

I'm not sure what "date type component" is, but in Features UI there is a dropdown 'Edit components' where I selected 'Date' as dependency. Even tried to select 'Date API' as dependency.

Using Strongarm I selected variable 'date_format_short_string' (my format name).

Still no luck - no implementation of hook_date_format_types().

Could you please describe the process of how this kind of feature would be created since, apparently, auto-discover does not pick up required components?

Thanks

thedavidmeister’s picture

For instructions on how to export date formats and date format types while this is getting fleshed out, check this duplicate thread #911940: Integration with Date (has a link pointing back here).

dalin’s picture

The patch in #15 worked for me. I included both the date format types and the strongarm variables date_format_[format] in my feature and I successfully reverted the feature on the destination site and everything is functional. The only issue being that the formats don't appear at /admin/config/regional/date-time/formats because nothing exists in {date_formats} because nothing is added to the feature. This is probably not within the sphere of Features module since {date_formats} has numeric IDs.

rballou’s picture

I think it would be great for features to add support for date formats and date format types. I do think it would be ideal if the integration could get around the numeric ID issue in some way but I'm not sure the best approach there. Are there any strategies we could take to get this functionality added? I can certainly help review or add code as needed. Thanks!

derhasi’s picture

@alex.designworks, there should be a "Date format types" component.

@dalin, rballou,I unfortunately don't get the numeri id problem :/

dalin’s picture

Numeric IDs is the reason that Features module does not manage blocks, menus or taxonomy either (@see Features Extra module). The reason being that if for example you enable a feature on your site and the ID for the item (block/menu/term/whatever) already exists, there's no way to know if this matches the item in your feature (and thus it should be updated), or if this is another item that was created on that site (and thus a new item should be created).

thedavidmeister’s picture

Example:

You have site A and site B.

You create three taxonomy terms (or anything else with a numerical ID) in A with ID's 1, 2 and 3.

You create a representation of the term with ID 2 in code (Features) that you want to push to site B.

Site B could have no terms, or 50 terms or anything else.

When you push the term to site B is it supposed to update (override) the term in the database that already has an ID of 2 or is it supposed to create a new term with those attributes with id 50?

In the case where ID's are created sequentially in the system, there's no "intention" of the developer/site admin we can infer and in the case where ID's can be freely edited by site admins/content managers (like term names) developers don't want to accidentally override something they aren't supposed to be touching while deploying new functionality.

Not an issue when you're creating a new site from scratch because everything is new, is an issue in the general case where you're using Features to deploy things in an ongoing fashion to existing sites that are already full of content.

It's possible to give *everything* a universally unique id (called a UUID) and reference that so you "create it if it doesn't exist, update the thing we exported and don't touch anything we don't know about" but it turns out to "guarantee" that a unique ID is really unique we end up with very long hashes and pulling results out of a database by referencing big, random numbers/strings is much slower than sorting/finding something indexed with a sequential ID. I think we can all agree that the last thing Drupal needs is *another* fundamental part of its "best practice" framework substantially slowing down page load times.

Things that have an unchangeable "machine id" that is set by a human's conscious decision when the thing is created are also easy to export because it is always "update if it exists, create it if it doesn't" which is "revert" in Features-speak. This is the current "best" approach to export things.

The Features-way of doing things is in core in Drupal 8 so the problem of deploying *functionality* (not content, yet, that's happening here - http://drupal.org/project/deploy) should dissipate completely over the next few years but at the moment it's not always obvious how to upgrade everything to the new paradigm without data loss for existing sites.

There's also no "one size fits all" solution that can be posted in a wiki where maintainers can reference it because every module is doing things in its own way to some extent - the double edged sword of open source innovation is that some innovations progress the quality and state of the platform and some end up being counter-productive.

This is the Numeric ID problem.

mpotter’s picture

Status: Needs review » Needs work

Because of the issues in #17 this needs more work. Also I am unsure about marking this as the implementation of the "system" module. There are a lot of other things controlled by "system" that this does not export. So it's a bit different than other core Features like "fields".

My feeling is that this really needs to go into a contrib module like Features Extra did and isn't going to be an easy addition to core Features.

kclarkson’s picture

Are custom date formats going to be implemented in the 7.x-2.x version ?

nevergone’s picture

And now?

discipolo’s picture

as mentioned this patch exports date types correctly. if i understand correctly the ability to add date formats is out of this modules scope and should maybe be investigated in the features_extra issue queue
edit: i just realized that it is possible to export date formats once the locale module is enabled. so maybe the numbered id issue doesnt apply after all (though the formats still dont import, not even with locale). i was just about to try out http://drupal.org/sandbox/rballou/1699026 ...

rballou’s picture

That sandbox module you point to will get both, but it's "ignoring" the numeric ID issue. It's intention was to work in a case where we can assume the feature was enabled when first installing the site. It's not necessarily robust for full usage. My understanding is too many other things point to format ID numbers...

pinkonomy’s picture

After applying this patch,I can see the Date formats,but not the Date types.Any suggestions?

thedavidmeister’s picture

tobiasb’s picture

What is the decision? Include it in features, features extra or a own project?

I build a own module for that with the components date types, localized format and custom formats (without ID ;-)).

I created a gist for you to take a look at the code ;-).

https://gist.github.com/tobiasbaehr/7292175

ladybug_3777’s picture

Issue summary: View changes

I'm also curious what the decision is for this. It hasn't been updated in 6 months, is this closed and won't be fixed/addressed?

discipolo’s picture

that gist looks like it could be turned into a patch for features_extra easily ... does that code work without the most recent patch from this queue?

tobiasb’s picture

Yes it works without the patch from here, because I used some code fragments from the patch.

joris_lucius’s picture

@tobiasb Thanks a lot, #33 worked.

yingtho’s picture

Title: Features support for date_formats » Features Extra support for date_formats
Project: Features » Features Extra
Status: Needs work » Needs review
FileSize
11.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features_extra-add-date-format-support-1279928-38_0.patch. Unable to apply patch. See the log in the details link for more information. View

I moved the issue to Features Extra and include a patch as well that is inspired from #33 but aligned with the naming convention of feature extra.

Status: Needs review » Needs work

The last submitted patch, 38: features_extra-add-date-format-support-1279928-38.patch, failed testing.

Status: Needs work » Needs review
samhassell’s picture

Status: Needs review » Needs work

Patch applies fine, date strings and formats seem to export fine, but when reimported they are stuck in the overridden state.

The last submitted patch, 38: features_extra-add-date-format-support-1279928-38.patch, failed testing.

The last submitted patch, 2: features-date_formats-1279928-2.patch, failed testing.

The last submitted patch, 3: features-date_formats-1279928-3.patch, failed testing.

The last submitted patch, 5: features-date_formats-1279928-5.patch, failed testing.

The last submitted patch, 6: 0001-Support-for-cores-date-formats.patch, failed testing.

The last submitted patch, 10: features-date_formats-1279928-10.patch, failed testing.

The last submitted patch, 15: features-date-1279928-15.patch, failed testing.

RoSk0’s picture

Status: Needs work » Needs review
FileSize
11.93 KB
PASSED: [[SimpleTest]]: [MySQL] 81 pass(es). View
8.53 KB

Fixed issue with reverting date types plus small code style fixes.

lklimek’s picture

I've installed patch from #49 and it seems to work.

aitala’s picture

I've installed patch #49 and fe date does not appear in the module list. Tried clearing cache, registry rebuild, cron, etc..

Eric

JvE’s picture

@aitala: The patch has to be applied to the features_extra directory (in sites/all/modules/).
I suspect you applied the patch to the drupal root which placed the 2 files there. If so, then simply move the 2 files (fe_date.info and fe_date.module) to the features_extra directory.

aitala’s picture

@JvE : Actually, the files were in the features_extra directory... just did not work.

I ended up just not using the module as a whole...

Eric

drupov’s picture

Status: Needs review » Reviewed & tested by the community

Patch from #49 applies perfectly and works great. Thanks!

markus_petrux’s picture

Ditto. Thanks!

stefan.r’s picture

Tested this and this still works like a charm!

  • pfrenssen committed dcf9ad9 on 7.x-1.x authored by derhasi
    Issue #1279928 by derhasi, RoSk0, mrfelton, yingtho: Add support for...
pfrenssen’s picture

Great stuff! Fixed some nitpicks and committed to 7.x-1.x. Thanks a lot everybody!

  1. +++ b/fe_date.module
    @@ -0,0 +1,353 @@
    +  return ($format) ? $format : NULL;
    

    No need for parentheses here.

  2. +++ b/fe_date.module
    @@ -0,0 +1,353 @@
    +  return ($format !== FALSE) ? unserialize($format) : NULL;
    

    These parentheses are also not needed, the comparison operator has a higher precedence than the ternary operator.

pfrenssen’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

Bobík’s picture

Btw, it will be good to mention this feature on the project page.

ciss’s picture

In our case clearing the cache was required for a custom type to show up, even after a forced revert. Just mentioning it in case someone else runs into this problem.