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.
Comment | File | Size | Author |
---|---|---|---|
#29 | 2633870-29.third-party-defaults.patch | 16.96 KB | jonathan1055 |
|
Comments
Comment #2
jonathan1055 CreditAttribution: jonathan1055 commentedIn #2490574: Validate that the publication date is in the future on comment 15 I suggested that we use constants to set the default values.
and then
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
Comment #3
jonathan1055 CreditAttribution: jonathan1055 commentedIn #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.
Comment #4
jonathan1055 CreditAttribution: jonathan1055 commentedHere'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).
Comment #6
pfrenssenThese two should remain booleans, it makes no sense turning them into integers, it's either on or off.
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()
).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.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.
Comment #7
pfrenssenJonathan, 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.
Comment #8
jonathan1055 CreditAttribution: jonathan1055 commentedHi Pieter,
Thanks for your review. In response:
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.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?Comment #9
pfrenssenSchedulerApi::MY_CONSTANT
.Comment #10
pfrenssenUpdated issue summary with expanded scope.
Comment #11
pfrenssenGoing to work on this.
Comment #12
pfrenssenHave 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:
Comment #13
pfrenssenHere's an updated patch.
SCHEDULER_DEFAULT_PUBLISH_PAST_DATE
. Most of the options were straightforward, but for the expansion of the fieldset I opted forwhen_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 sayingwhen_required_or_field_has_data
. Is this OK?use_vertical_tabs
option tofields_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 nowSCHEDULER_DEFAULT_FIELDS_DISPLAY_MODE
.testExtraFields()
totestFieldsDisplay()
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.Comment #15
jonathan1055 CreditAttribution: jonathan1055 commentedThis 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 intestNonEnabledNodeType()
is already fixed in patches which have not been committed yet.Comment #16
jonathan1055 CreditAttribution: jonathan1055 commentedThe 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.
Comment #17
pfrenssenRerolled. The additional test coverage uncovered another bug: the fieldset was not correctly expanded when either the
publish_on
orunpublish_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:Comment #18
jonathan1055 CreditAttribution: jonathan1055 commentedha 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.
Comment #20
jonathan1055 CreditAttribution: jonathan1055 commentedI'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?
Comment #21
jonathan1055 CreditAttribution: jonathan1055 commentedFound the problem - the actual form field was not re-named:
was not changed and should have been
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.
Comment #22
pfrenssenMy bad! Good that you found it. It's looking great now!
Comment #24
jonathan1055 CreditAttribution: jonathan1055 commentedYes, 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.
Comment #25
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI'm going to work on this, and define constants for the remaining default values.
Comment #28
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHere'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.
Comment #29
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedNow 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.
Comment #31
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI 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<?php>
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.
Comment #32
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedUnassigning myself.