In our current codebase we have ten third-party settings for node types, and the when using getThirdPartySetting the correct default value needs to be supplied in parameter 3. In many of the cases the default we want is FALSE so on the occassions where parameter 3 for getThirdPartySetting has been missed we get away with it. But in this is not always the case, and we should have a better more-consistent method where the actual default values can be defined once, instead of replicating the values and strings throughout the files.

Also, in some cases we are using a boolean where we should use a different type. For example, we currently allow to display the scheduling fields as either a fieldset or vertical tabs and this is stored as a boolean: use_vertical_tabs: on/off. This is not future proof, we might want to extend these options in the future, and people may want to add their own custom display options to the list. Let's change the cases where the choice is not strictly binary to integers, and have a look at the names of the settings.

Attached is a full search of all uses of getThirdPartySetting. There are some anomolies which we need to sort out.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

In #2490574: Validate that the publication date is in the future on comment 15 I suggested that we use constants to set the default values.

// Specify defaults for the content-type default settings, which should be used
// as a consistent third parameter for getThirdPartySetting() calls.
define('SCHEDULER_DEFAULT_PUBLISH_PAST_DATE', 'error');

and then

$type->getThirdPartySetting('scheduler', 'publish_past_date', SCHEDULER_DEFAULT_PUBLISH_PAST_DATE)

This avoids hard-coding the third parameter values in all the calls, and ensures we always have a consistent default.


edit: This change has been committed http://cgit.drupalcode.org/scheduler/commit/?id=e84a3b3
jonathan1055’s picture

In #2645818: Date input fieldset expand/collapse is not working we discussed the radio setting options and the fact that the default form values were not reflecting the settings. Here's a patch which makes the settings consistent within the module and uses integer instead of boolean so that the radio #default_value is correct.

jonathan1055’s picture

Here's the same patch but with the changes also made to the .test file (for this patch, I also removed testNonEnablesNodeType, as this is moving to a different class now, and the test would not complete if left in).

Status: Needs review » Needs work

The last submitted patch, 4: 2633870-4-vertical-tabs-expand-fieldset-defaults.patch, failed testing.

pfrenssen’s picture

  1. +++ b/config/schema/scheduler.schema.yml
    @@ -55,8 +55,8 @@ node.type.*.third_party.scheduler:
         use_vertical_tabs:
    -      type: boolean
    +      type: integer
           label: 'Display the scheduling fields in vertical tabs'
         expand_fieldset:
    -      type: boolean
    +      type: integer
           label: 'Always expand the scheduler date input fieldset'
    

    These two should remain booleans, it makes no sense turning them into integers, it's either on or off.

  2. +++ b/scheduler.module
    @@ -32,6 +32,8 @@ define('SCHEDULER_TIME_LETTERS', 'hHgGisaA');
    +define('SCHEDULER_DEFAULT_USE_VERTICAL_TABS', '1');
    +define('SCHEDULER_DEFAULT_EXPAND_FIELDSET', '0');
    

    Any particular reason you want to put these in constants? I'm not necessarily against it, but in core they are not using constants, but rather the values directly (see for example in ContentTranslationManager::isEnabled()).

  3. +++ b/scheduler.module
    @@ -505,7 +507,7 @@ function scheduler_field_extra_fields() {
         $publishing_enabled = $entity->getThirdPartySetting('scheduler', 'publish_enable', 0);
         $unpublishing_enabled = $entity->getThirdPartySetting('scheduler', 'unpublish_enable', 0);
    -    $use_vertical_tabs = $entity->getThirdPartySetting('scheduler', 'use_vertical_tabs', 1);
    +    $use_vertical_tabs = $entity->getThirdPartySetting('scheduler', 'use_vertical_tabs', SCHEDULER_DEFAULT_USE_VERTICAL_TABS);
     
         if (($publishing_enabled || $unpublishing_enabled) && !$use_vertical_tabs) {
           $fields['node'][$type->type]['form']['scheduler_settings'] = array(
    

    hook_field_extra_fields() doesn't exist any more in Drupal 8, we are using base fields now. This whole function can be removed. Perhaps it's better to do this in a separate commit, since this is unrelated to this issue.

  4. +++ b/src/Tests/SchedulerFunctionalTest.php
    @@ -512,77 +512,4 @@ class SchedulerFunctionalTest extends SchedulerTestBase {
    -   /**
    -   * Tests that a non-enabled node type cannot be scheduled.
    -   *
    -   * This checks that the scheduler input date/time fields are not displayed
    -   * if the node type has not been enabled for scheduling.
    -   */
    -  public function testNonEnabledNodeType() {
    

    This test deletion is not related to this issue, you mentioned that this needs to be moved, maybe just do that in a separate commit.

pfrenssen’s picture

Jonathan, I just saw your comment #2645818-3: Date input fieldset expand/collapse is not working that explains why you changed the values to integers. I answered it in comment #2645818-13: Date input fieldset expand/collapse is not working.

jonathan1055’s picture

Hi Pieter,
Thanks for your review. In response:

  1. Saw your comment and have replied in #2645818-14: Date input fieldset expand/collapse is not working
  2. I thought of using constants so that any place where getThirdPartySetting is used, the programmer can rely on the constant to provide the correct default value. As I explained in the summary at the top of the issue, our current code-base has a mix of default values (see the text file). Also in recent fixes/code corrections, the wrong default was used which lead to extra testing, wasted time and unnecessary patch re-rolls. Also, should we wish to change the default value in future we can change it in one place for consistency throughout. I don't think it should be up to our programmers to have to remember what the correct default value is, or to have to search for the file where the form is defined and deduce the default. It just makes it easier for everyone. Plus is creates have one code location where we can see all the default definitions in one place.
  3. Yes I did wonder if hook_field_extra_fields() was still used. I made the change there, as I did not want to leave it out. Separate commit definitely. Do we need a separate issue to go through all functions which are now redundant?
  4. Yes, the removal of the test to a different class is covered in #2594615-111: Automated testing in 8.x [meta]. I just wanted to temporarily remove it here, to see how the tests performed. In my local testing (in main branch not testing branch) this test had a fatal error and did not complete, so I just wanted to get rid of it here, so that the remainder of the tests ran.
pfrenssen’s picture

  1. I agree with your suggestion. Let's change it to integers so we are ready for future expansion of the options, and allow developers to add in additional options using a simple form alter.
  2. Ok, agreed. We should at some point in the future take the constants out of the global space, and put them in a class, so we don't pollute the global space, and can use them like this: SchedulerApi::MY_CONSTANT.
  3. I'll remove that in a separate commit. I'll also look for other obsolete functions in the main module file. I believe we still have Views + CTools API hooks, maybe more.
  4. That makes sense, thanks for clarifying!
pfrenssen’s picture

Title: Consistent defaults for third party settings » Revisit defaults for third party settings
Issue summary: View changes

Updated issue summary with expanded scope.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Going to work on this.

pfrenssen’s picture

Have cleaned up the module file, I have removed all the obsolete functions and have added todo's wherever I saw that some D8 porting work remains to be done.

I just saw that we used strings for our other multi-value options instead of integers, that actually makes a lot of sense and improves readability of the code:

define('SCHEDULER_DEFAULT_PUBLISH_PAST_DATE', 'error');
pfrenssen’s picture

Here's an updated patch.

  • Replaced the integers with strings to be more readable and consistent with SCHEDULER_DEFAULT_PUBLISH_PAST_DATE. Most of the options were straightforward, but for the expansion of the fieldset I opted for when_required, while this is not 100% true: the fieldset will be expanded when required, or when the field has data, but I couldn't find a short way of saying when_required_or_field_has_data. Is this OK?
  • Changed the naming of the use_vertical_tabs option to fields_display_mode so that it better matches the fact that it is no longer a boolean. Also reflected this in the name of the constant which is now SCHEDULER_DEFAULT_FIELDS_DISPLAY_MODE.
  • Renamed testExtraFields() to testFieldsDisplay() so it cannot be confused with the concept of "Extra fields" in the D7 field API.

I have not included the removal of testNonEnabledNodeType() in this patch. We can inspect the test results to see what exactly is failing.

Status: Needs review » Needs work

The last submitted patch, 13: 2633870-13.patch, failed testing.

jonathan1055’s picture

This is great! Yes I was thinking that text strings would be even better than integers. I'll test your patch but it all looks good at first glance.

Regarding your changes to testExtraFields() I've already got a patch which expands that test to cover more cases which were not checked. I will incorprate your name change there. The fatal error in testNonEnabledNodeType() is already fixed in patches which have not been committed yet.

jonathan1055’s picture

The patch will need re-rolling due to http://cgit.drupalcode.org/scheduler/commit/?id=54d3793
Sorry about the clash. I can re-roll tomorrow if you do not get there first.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
8.82 KB

Rerolled. The additional test coverage uncovered another bug: the fieldset was not correctly expanded when either the publish_on or unpublish_on field contained data. I have no interdiff because the original patch could no longer be applied, but this is the relevant section in the patch that has changed:

@@ -128,9 +130,13 @@ function scheduler_form_node_form_alter(&$form, FormStateInterface $form_state)
   // If either publishing or unpublishing is enabled, provide a field group to
   // wrap the scheduling fields.
   if ($publishing_enabled || $unpublishing_enabled) {
-    // Expand the fieldset if either publishing or unpublishing is required, or
-    // if the fieldset was configured to be expanded.
-    $expand_details = $publishing_required || $unpublishing_required || $type->getThirdPartySetting('scheduler', 'expand_fieldset', FALSE);
+    // Expand the fieldset if either publishing or unpublishing is required, if
+    // the fieldset was configured to always be expanded, or if the fields
+    // already contain data.
+    $always_expand = $type->getThirdPartySetting('scheduler', 'expand_fieldset', SCHEDULER_DEFAULT_EXPAND_FIELDSET) === 'always';
+    $has_data = !empty($node->publish_on->value) || !empty($node->unpublish_on->value);
+    $is_required = $publishing_required || $unpublishing_required;
+    $expand_details = $always_expand || $has_data || $is_required;
jonathan1055’s picture

ha ha, actually the test did not reveal the bug, I spotted that the functionality had been dropped and reported it in #2645818-8: Date input fieldset expand/collapse is not working. Then I wrote the tests to make sure it does not get forgotten or missed again.

I think that the fix for expanding for existing dates should be done in #2645818: Date input fieldset expand/collapse is not working as that is the more relevant issue. Your new code looks good but it belongs in that issue. This issue should remain solely about the third-party defaults. Here is your patch from #13 without any code changes to schema.yml, .admin.inc or .module, but just with the re-roll necessary for testFieldsDisplay().

I've just done a quick manual test, and the config inspector module still shows the old variable name 'use_vertical_tab'. However, this might clear itself with a cache rebuild, but it did not do so immediately. Also the value of 'fields_display_mode' shows as empty in config inspector, although the admin form appears to be working OK.

Let's get this issue finalised and committed, before we do anything more on the expanded fieldset, as once we have the new schema in place and confirmed it will be easier to move forward.

Status: Needs review » Needs work

The last submitted patch, 18: 2633870-18.third-party-defaults-and-schema-change.patch, failed testing.

jonathan1055’s picture

I've done further manual testing, and expand_fieldset is working fine. The values are saved, the form default is redisplayed correctly and the correct action is taken when editing a node.

However, there is still something wrong with the newly renamed fields_display_mode. When I revisit the content type form the radio for 'vertical tab' is always showing, regardless of what was selected on the previous save. The value is not stored and the config inspector shows <empty>. I have cleared the cache via devel performance which did not fix it. I ran Devel 'reinstall modules' which calls hook_uninstall and hook_uninstall, still not fixed. I uninstalled the entire module, and reinstalled - still with no improvement. The code changes look fine, so something is stopping the new schema from being recognised, thus preventing the new text string from being saved. Any ideas?

jonathan1055’s picture

Found the problem - the actual form field was not re-named:

  $form['scheduler']['node_edit_layout']['scheduler_use_vertical_tabs'] = array(

was not changed and should have been

  $form['scheduler']['node_edit_layout']['scheduler_fields_display_mode'] = array(

Manual testing all fine now. No re-install necessary, just visit the content type form and save, which is what I thought should be the case.
New patch attached with this one line fixed.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

My bad! Good that you found it. It's looking great now!

  • jonathan1055 committed 6528a79 on 8.x-1.x authored by pfrenssen
    Issue #2633870 by jonathan1055, pfrenssen: Change boolean to string for...
jonathan1055’s picture

Status: Reviewed & tested by the community » Active
FileSize
296.75 KB

Yes, this is a good change. Thanks for you work on it, I am happy that we did this. I added a @todo in .module to store your suggestion about moving the constants out of global space, but that can be a separate issue.

When you next save the content-type form the new schema values are stored correctly. However, we still get left with the warning that the schema for 'use_vertical_tabs' is missing (see image). As this is only in development it is ok, and I think on a re-install that would get tidied up, but do you know how to clear/reset/reload the schema from code and thus remove this warning?

I've set this back to active, because I think we still need to discuss the defaults for the other third-party settings. In the original summary at the top I mentioned that there are 10 settings. We now have 3 with string values and defined default values, but the remaining 7 booleans have inconsistent defaults. Some of the calls to getThirdPartySetting use '0', some use FALSE and some omit the 3rd parameter altogether. The code is working but it could be cleaner, and hence easier for future understanding/maintenance.

Should we go all the way and define default constants for the remaining booleans? That would make the code completely consistent and then we'd be less likely to introduce anomolies. All the defaults would be readable in one single code location, which I think would help us if we have to make changes, and also when deciding how two settings may interact.

jonathan1055’s picture

Assigned: Unassigned » jonathan1055

I'm going to work on this, and define constants for the remaining default values.

The last submitted patch, 17: 2633870-17.patch, failed testing.

Status: Active » Needs work

The last submitted patch, 21: 2633870-21.third-party-defaults-and-schema-change.patch, failed testing.

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
16.11 KB

Here's a patch to set default constants for the other seven settings. This has allowed me to run the tests with different default values in turn, and I have made a few minor changes to the tests and assertion texts to make it easier to understand when a test fails. Interesting, it showed that one setting 'PUBLISH_TOUCH' was not actually being tested explicitly, so I will add test coverage for that.

I have created a spin-off issue #2682579: Move constants out of global space into settings and schema to deal with that, after this issue is complete.

jonathan1055’s picture

Now that #2667692: Re-instate field_extra_fields for 8.x is committed, there are two more calls where FALSE needs to be changed to the constant. Added these to a new patch.

  • jonathan1055 committed 8cc62c1 on 8.x-1.x
    Issue #2633870 by jonathan1055, pfrenssen: Constants as defaults for...
jonathan1055’s picture

Status: Needs review » Fixed

I decided to commit this. As per Pieter's comments on #2625572-37: Admin setting 'Enable content type for scheduler' is ignored we need to keep things moving and this issue, whilst being important, is also not contentious. We already had constsants for three of the default settings, so extending to the remaining seven settings makes sense, and needs to be done before too many more patches are produced. Now is a good time.


Original 26th March
I just noticed the new styling for <code> and &lt;?php&gt; tags! The coloured keywords are good, but the black background stands out as such a stark contrast. That will take a bit of time to get used to.
...
Update 2nd April
Seems that others did not like the black code background either, and now it has changed to a pale grey - much easier on the eye.
jonathan1055’s picture

Assigned: jonathan1055 » Unassigned

Unassigning myself.

Status: Fixed » Closed (fixed)

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