Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Suggested commit message
git commit -m 'Issue #2927563 by amateescu, tstoeckler: Aggregator feed \''refresh\'' field should have a default value'
Problem/Motivation
The refresh (Update interval) field of aggregator feeds is required but does not have a default value. That is not necessarily a problem, but it requires any consumer to explicitly fill this field whenever a feed is created. With #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field this will also be enforced on the storage-level so that even when creating feeds via the API it will not be possible to omit a value for it.
Because this field only takes a limited set of allowed values, however, (in contrast to, for example, the node title) it seems fairly natural to pick a default value. Currently the default value is set on the form layer, but it seems unfair that Rest clients have to pick a value explicitly. (Because this issue resolves a disparity between forms and Rest clients, tagging as related to the "API-First Initiative".)
Proposed resolution
Add a default value to the field and remove the setting of the default value from the form layer.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | Screenshot_20171206_212522.png | 21.14 KB | amateescu |
| #2 | 2927563-2.patch | 3.5 KB | tstoeckler |
Comments
Comment #2
tstoecklerHere we go.
The interdiff is relative to the patches in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field. I added an update path test. I also changed the default value to 3600 as that is what the form uses and removed the code from the form layer. That will not only make API-first people happy, but will also make it easier for us if we ever get around to converting feeds to use a route provider.
Comment #3
jibranUpdate hook look goods we also have an upgrade path with the test so this is ready.
Comment #4
jibranThis is major as it is blocking #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.
Comment #5
amateescu commentedThe patch looks good to me, I agree that using the default value set by the form is better than the one mostly used in tests (
3600vs.900).Also, it would be nice to not forget to credit the author of the patch from #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field where this problem was initially discovered and patched :)
Comment #6
tstoecklerYes, very good point, thanks for pointing that out: So that would be you or do you suggest anyone else that should be credited additionally?
Added a suggested commit message, feel free to adapt if I forgot someone. Also changing title to use double-quotes it seems the commit message generator works better with that? Not sure...
Comment #7
tstoecklerComment #8
tstoecklerHmm... mucking about with the commit message still, d.o. is being weird. Sorry for the spam...
Comment #9
wim leersNaïve question: shouldn't existing entities also be updated to have this default value, if they don't have a value today?It's a required field, never mind! 👍
Comment #10
xjmCan we get before-and-after screenshots for this?
Also, saving issue credit.
Comment #11
amateescu commented@xjm, this patch does not introduce any visual change, it simply moves the default value from the code that generates the "add feed" form to the base field definition of the 'refresh' field.
Here's how it looks like before and after the patch. The part that we're dealing with here is the 'Update interval' field, which has the same default value before and after the patch.
Comment #13
tstoecklerComment #14
larowlanCrediting @amateescu as per comments
Comment #17
larowlanCommitted as 8da25f6 and pushed to 8.5.x
Even though this is only a task, because it blocks a major bug in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field - I cherry-picked as6372cc3 and pushed to 8.4.x