Date input fieldset expand/collapse is not working

Hi Joe,
Thanks for looking at this. First, regarding the setting not being saved, I think you are mistaken. The setting is being saved but the value is not being reflected properly in the default that is set when the form is displayed again. So it just appears not to be saved, whereas it is a processing fault in our form. I had noticed this when analysing #2633870: Revisit defaults for third party settings. The missing default is probably to do with using a boolean to store the value, which gets returned as 1 or NULL, but using a radio-selection to display it on the form, which needs 1 or 0 to show the correct button highlighted in this case. As you correctly noticed, if the selection is 'vertical tab' then it does stay selected, but if it is 'fieldset' then the next time you go to the form neither buttons are highlighted. This is because the '1' is being used to set the default, which works. If we want to use a radio selection then we should probably be storing the values as integers, so that if the radio options increase in future it will simply just work without any more changes. We've been getting away with this because we only have two options at present. The alternative would be to fiddle with the values when defining the default in the form, but that is clumsy.

For background on the change to the schema see #2594615: Automated testing in 8.x [meta] comment 66. In doing that work, I discovered the excellent config_inspector module which shows the actual values for the scheudler settings per content type, exactly as they are stored, i.e. does not let the admin form get in the way and mislead us.

Regarding the fieldset problem, I think it's because of other code that added a check for vertical tabs.

Yes as I mentioned in the issue summary #2597233: The FALSE value for #group FAPI property does not have expected behaviour did affect the processing, but I recall that it might not have been working even before that. But certainly that code is now not doing the right thing, because it only uses $expand_details (a fieldset option) when $use_vertical_tabs is also on.

should I update these in this issue or create another one? Do you think it would effect people using this version of the module, if there are any?

I'll make a patch for the schema/default change and put it in #2633870: Revisit defaults for third party settings as it is related to that issue. No, it should not affect anyone in a serious way because (a) the code does not work at the moment anyway, (b) as soon as they use the admin form the values will correct themselves, and (c) we have not made an alpha or beta release version yet, so OK that these settings can change.

I'll leave this issue assigned to you, so you can work on the vertical tabs/fieldset problem.

Thanks. We now get the fieldset back, which is good. The setting 'Always open the fieldset, even if no dates exist' now works fine, but 'Expand only when a scheduled date exists or when a date is required' does not cause the fieldset to expand when a date is present. The setting of $expand_details needs to check if dates are set. The code in 7.x was:

  $fieldset_extended = (
    (isset($defaults->publish_on) && $defaults->publish_on != 0)
    || (isset($defaults->unpublish_on) && $defaults->unpublish_on != 0)
    || $publishing_required
    || $unpublishing_required
    || variable_get('scheduler_expand_fieldset_' . $form['type']['#value'], 0)
  );

but it seems we have lost the date checks on converting to 8.x. They need to be put back in.

We also need to verify that the changes in this patch do undo the original intent from #2597233: The FALSE value for #group FAPI property does not have expected behaviour.

I've added a patch to #2633870: Revisit defaults for third party settings which conflicts with this one. I think it would make testing easier here if that patch was committed first.

Also, just noticed that because you have removed the entire check if ($use_vertical_tabs && ($publishing_enabled || $unpublishing_enabled)) then we now get scheduler options for non-enabled content types. Maybe we should check this further up the function? If neither are enabled then simply RETURN and do nothing in this function. I don't know why that is not done already?

Or was that the problem in #2597233: The FALSE value for #group FAPI property does not have expected behaviour? I need to go back and re-read that issue more closely.

since this was still working recently

It was affected by that issue, but I am fairly sure it was not entirely working correctly before.

It wouldn't be bad to add a test so this won't regress again.

We do have tests for this, and they are currently failing :-)

Hi Pieter,
Thanks for looking at this, and making the new patch. Great work on allowing the 'expand' option for the vertical tabs, that is a good improvement. I note that is is only relevent for themes that implement the advanced tabs. For the traditional vertical tabs it is not applicable, but that's ok, it does not cause any problems.

We should use the correct data types for storing the data ...

The reason for changing from boolean was not to match the radio values, it was the fact we were using radios that alerted me to the discrepancy between the storage and the usage. Radios, by definition, allow for more than two values and the change would save ourselves future effort. There may be a third display option which we want to incorporate, who knows what might be available in 8.1 or later, or there could be a new contrib module we could allow our users to integrate with, for example a jQuery pop-up to enter the data, which is neither a vertical tab nor a fieldset. I just thought that allowing for simple expansion would save us having to write a hook_update later to convert from boolean. With the 'expand' option, I think we should already offer three choices, by adding 'always collapsed'. I can imagine themes/layouts where the designer would always want the fieldset collapsed and just allow the user to open it when needed.

I thought it is worth having this discussion now. If you really want to stay with boolean, I'd be ok, just good to talk it through instead of leaving it too late.

Good. I will incorporate the change to integer and re-roll your patch from #12 for review. However, the actual commits could be done separately if we wish, as the change of schema should be separated from the fixing of the fieldsets expand/collapse.

Here's a patch to fix the 'expand if dates already exist' problem I reported in #8 above. The code is taken from Pieter's patch in #2633870-17: Revisit defaults for third party settings

Good, we get an extra class with all passes

✓		- testFieldsDisplay

Overall 13 pass, 6 fail, compared to 12 pass, 7 fail before.
Thanks guys.

Ignore the failed patches above. They were queued but not run until the 8.x committed codebase passed all tests. That happened with my commit a few minutes ago, and then the untested patches have suddenly come to life and been run. This issue is already fixed and closed.