I am trying to make this Date Popup Authored work together with http://drupal.org/project/override_node_options which lets the site administrator give users without "administer nodes" possibility to change specific options of the node, such as "authored on". However, when I enable "override [content type] authored on" for a specific user role they don't get the date picker, just the normal date field. Only users with "administer nodes" get the date picker.

Could this be because of module weights?

Feel free to move issue to override_node_options if you think it belongs there.

Thanks!

Comments

Mark Trapp’s picture

It's actually unrelated to whether a user has the administer nodes permission, but how Override Node Options works.

The fix for #1087616: Post date resets on save if user can't administer nodes included a check for $form['author']['#access']: #access being the Drupal Way to determine if a field is available.

A quick perusal of Override Node Options's code indicates it is attempting to set $form['author']['#access'] during the node form alter, which is the same time Date Popup Authored is checking to see if it should alter the form.

I do suspect it is an issue of module weights: I'm not familiar with the design decisions that went into Override Node Options to declare this is their bug, but since it's mucking about with #access, it seems it should be declaring its weight to be relatively high to always alter the form first.

There's nothing I can really do to support other modules like Override Node Options: Date Popup Authored replaces a core-provided field, so it assumes that field is unmodified. That is, when Date Popup Authored attempts to alter the form, either $form['author']['#access'] is set or it isn't.

I'm marking this as won't fix for Date Popup Authored, but it might be worth checking with the maintainers of Override Node Options to see if they are willing to set the module's weight. If not, you could always use the Util module to manually set the module's weight yourself.

LarsKramer’s picture

Title: Compatibility with module Node Override Options: only users with "administer nodes" see the date picker » Date picker not available when Node Override Options is enabled

Thanks for your quick reply. I have now verified that lowering the weight of Node Override Options (NOO) solves the issue. This makes sense to me since NOO should have a chance to modify access before DPA checks for this. And since they both have weight=0, by default DPA comes first due to alphabetical order.

I don't know what is more correct: raising the module weight of DPA or lowering the weight of NOO. There might be other implications. But if we can fix it in the the code of one the modules, I think it is a better solution than manually editing the system table (or using Util module). I am marking active again in case you should have any further comments, otherwise just close again :-) And I will ask the maintainers of NOO what they think is right solution.

Mark Trapp’s picture

You're right: lowering so Override Node Options comes first.

I apologize for not making it clear in my previous post why this is won't fix for Date Popup Authored. Date Popup Authored makes a change to a core-supplied field that more or less makes it incompatible with any other module. In order for it to function, it has to assume it's working with a functionally clean version of the Authored On field.

That is, Date Popup Authored doesn't and can't handle the myriad of ways a module, contrib or custom, has messed with the Authored On field: it has to assume the field is still playing by the rules. So there's no reason to modify its module weight: it's not supposed to come after (or before) any other modules.

Modules like Override Node Options are an interesting exception because they modify fields in a way such that the end result makes it indistinguishable to how core handles it. Modifying the #access attribute is the correct way to handle visibility for other modules who want to know if they can use the field.

And with the change in #1087616: Post date resets on save if user can't administer nodes, Date Popup Authored does check #access to see if the Authored On field is available before attempting to mess with it. Date Popup Authored modifies the field in all the instances where core would show the user the Authored On field, and correctly disables itself in all the instances where core wouldn't show the user the Authored On field.

The problem arises in that the change Override Node Options does make to normal core functionality is done too late for any module that happens to come before it in the alphabet (like Date Popup Authored) to be aware of it.

So there are three different options Override Node Options has in terms of its design intent:

  1. It's intended to be the only module that modifies those fields,
  2. It's intended to be only user-facing functionality, or
  3. It's intended to be a full replacement for core's visibility handling, and thus code-facing functionality

The first is Date Popup Authored's design intent: its functionality is so bound to the way the core field functions it has to know the field is unmodified or modified in a way that's invisible to it. In that case, not setting the module weight is correct and users just need to keep in mind that enabling multiple modules that modify the same core-supplied fields can lead to unexpected results.

The second would mean that Date Popup Authored and other modules that modify core-supplied fields should completely ignore anything Override Node Options does because the change is mostly cosmetic to the user. In this case, it should set its module weight relatively high to ensure it comes last.

The third would mean that Date Popup Authored and other modules should respect the visibility settings Override Node Options sets. In this case, it needs to set its module weight relatively low to ensure it always comes first.

Based on my limited familiarity with the module and its code, I would suspect the last case to be its design intent: with the minor module weight change, the module should function perfectly with any other module that relies on #access. Right now, however, that intent is not demonstrated in its code: if a module happens to come before it in the alphabet, it won't see its changes, and if it does, it will.

So to summarize, if the intent for Override Node Options is either of the first two cases, Date Popup Authored works as expected: it correctly ignores anything Override Node Options does. If it's the third case, there's a bug in Override Node Options that should be fixed there, not in Date Popup Authored, so that all modules which make the reasonable assumption that when they look at #access it's been set properly can be aware of Override Node Options's changes.

LarsKramer’s picture

Status: Active » Closed (won't fix)

Hi Mark, thanks for your thorough explanation. I think I agree with you. FYI, I posted an issue to "Override Node Options" (sorry about not getting it's name correct the first time) here:
http://drupal.org/node/1090696

Mark Trapp’s picture

Title: Date picker not available when Node Override Options is enabled » Date picker not available when Override Node Options is enabled

Ah, fixed. For what it's worth, it appears Override Node Options is mostly abandoned so you might want to consider applying to be a co-maintainer to ensure the fix gets in.

TripleEmcoder’s picture

Just as an FYI, the recent versions of these modules for 7.x seem to work together well without any tweaking.

Mark Trapp’s picture

Title: Date picker not available when Override Node Options is enabled » Date Popup Authored does not play nice when $form['authored']['#access'] is modified too late
Version: 6.x-1.1 » 7.x-1.x-dev
Assigned: Unassigned » Mark Trapp
Category: support » task
Priority: Minor » Normal
Status: Closed (won't fix) » Active

Reopening this issue because it can be fixed and worked around easily in 7.x. I've also duplicated #1611846: Receiving "You have to specify a valid date" error message to this issue.

The main issue, as I mentioned in #3, is that when a module modifies $form['authored']['#access'] after Date Popup Authored does its magic, there's no way for Date Popup Authored to know it doesn't have what it needs, causing form errors.

In 6.x, the only real way to fix this is to modify either Date Popup Authored's weight or the weight of the module that's acting on $form['authored']['#access'] late.

In 7.x however, there are two things that can be done:

1. Use hook_form_BASE_FORM_ID_alter()

Date Popup Authored should be implementing hook_form_BASE_FORM_ID_alter() instead of hook_form_alter. This should be done anyway as it makes the code a bit cleaner, but the benefit to this issue is that hook_form_BASE_ID_alter() is invoked after any hook_form_alter() implementations. If you're modifying $form['authored']['#access'] in a standard hook_form_alter(), this will fix the problem.

An update with this change is forthcoming, once I sort out Date Popup Authored's tests.

2. Use hook_module_implements_alter().

Modules that are also implementing hook_form_BASE_FORM_ID_alter() can implement hook_module_implements_alter() to push Date Popup Authored after their invocation, also fixing the problem. An implementation might look something like this:

function mymodule_module_implements_alter(&$implementations, $hook) {
  if ($hook === 'hook_form_alter') {
    $group = $implementations['date_popup_authored'];
    unset($implementations['date_popup_authored']);
    $implementations['date_popup_authored'] = $group;
  }
}

This would push Date Popup Authored's hook_form_alter() implementation to always be last, solving the problem even if another module implements hook_form_BASE_FORM_ID_alter() to modify $form['authored']['#access'].

I won't add a hook_module_implements_alter() implementation to Date Popup Authored for the same reasons mentioned in #3: I don't want to make the assumption that site builders and developers don't know what they're doing by forcing Date Popup Authored to be last in all environments just to solve a single edge case. I don't think having site builders or developers who happen to be affected by it implement mymodule_module_implements_alter() is too big of an ask, and it doesn't require upstream changes to other contrib projects (like the Override Node Options case that spawned this issue).

Mark Trapp’s picture

Status: Active » Needs work

I've committed the change from hook_form_alter() to hook_form_BASE_FORM_ID_alter() without the tests just to get it out there: I'll still need to add tests before I roll a proper beta release. For now, you can use the development snapshot if you need this change.

barraponto’s picture

Yeah, we developers can implement hook_module_implements_alter but leaving the snippet linked in KNOWNISSUES.txt will just solve it :D

rwinikates’s picture

For the record, there is a patch in the linked issue at Override Node Options that works, and hopefully will be committed soon. http://drupal.org/node/1090696#comment-5674216

Mark Trapp’s picture

Assigned: Mark Trapp » Unassigned
Issue summary: View changes
Mark Trapp’s picture

Status: Needs work » Fixed

Tests are now fixed.

Status: Fixed » Closed (fixed)

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