Problem/Motivation
The "RSS publishing" settings form allowed users to change basic RSS feed behaviours on Drupal prior to version 8. Drupal 8/9 now uses a view to provide the front page /rss.xml feed link. This view does not use the RSS form settings, which is confusing for users.
Proposed resolution
Remove the feed description and number of items settings from admin/config/services/rss-publishing form, as neither of these actually have any effect anywhere.
The feed content setting may still be used by Views, this is more complicated to unwind and will be removed in #2601030: Views RSS view mode settings are completely broken.
Remaining tasks
None
User interface changes
The feed description and number of items settings are removed.
API changes
None
Original report by @cafuego
Comment | File | Size | Author |
---|---|---|---|
#94 | 2409413#comment-13910940.patch | 538 bytes | alex.mazaltov |
#87 | RSSpublishingSettingsForm-2409413-87.patch | 9.02 KB | SivaprasadC |
#80 | RSS-Feed-Review-2.PNG | 16.83 KB | SivaprasadC |
#80 | RSS-Feed-Review-1.PNG | 43.73 KB | SivaprasadC |
#77 | interdiff.2409413.75-77.txt | 645 bytes | longwave |
Comments
Comment #1
cafuego CreditAttribution: cafuego commentedComment #2
cafuego CreditAttribution: cafuego commentedComment #3
cafuego CreditAttribution: cafuego commentedAttached patch removes the RSS publishing settings form, its variables and its tests, the menu links, the RSS publishing migrations.
Comment #4
mradcliffeDoes this need to change to a view migration?
Comment #5
cafuego CreditAttribution: cafuego commentedI don't know, what's a view migration? ;-)
Comment #7
cafuego CreditAttribution: cafuego commentedRemoved superfluous (pending decision on whether a view migration is needed) test.
Comment #10
cafuego CreditAttribution: cafuego commentedMake patch apply.
Comment #11
jibranThis is RTBC IMO but do we need a change record for that.? We should ping some form migration team to eyeball that patch.
Comment #12
dawehnerShould we migrate those settings to the appropriate views settings or just drop it entirely as the patch does?
Comment #13
cafuego CreditAttribution: cafuego commentedI'm happy either way, but I have no idea how to migrate anything to a views setting :-/
Comment #14
dawehnerTheoretically we could but I doubt anyone cares about those settings anyway, especially because it was also used for taxonomy/term/.../feed afaik
Comment #16
cafuego CreditAttribution: cafuego commentedUpdated the patch, should apply again and hopefully pass testing.
Comment #17
jibranHere we go again.
Comment #20
cafuego CreditAttribution: cafuego commentedRemoved the exceptioning test, restored the generated Table files, so they don't mismatch on checksum.
Comment #21
dawehnerWell yeah to be fair we should not manipulate the data coming from D6. We can't really deny the existence of it.
Comment #22
xjmNice, I just noticed today that this form still existed in #2578995: Update the link descriptions on the Configuration page for the system module.
I think we need an upgrade path here to delete the stale
system.rss
config, though.Also, a UI that does nothing is a WTF and it doesn't break anything to remove a thing that does nothing, so tagging this as an RC target.
Comment #23
xjmComment #24
xjmI guess maybe this should also have a followup for migrating the settings to the View config, but Views migration is still under development.
Comment #25
catchJust re-titling because it sounded like a bug that it could be removed, not a feature ;)
Comment #26
alexpottI'm not sure that this issue is correct. This form also configures the view mode used by
Drupal\node\Plugin\views\row\Rss
. In order to remove system.rss completely we need to store the view mode with each view. In fact there is code hard coded to the fake rss display modes...Comment #27
alexpottThis is super complex because the form is configuring a kind of default view mode (it is not a real entity view mode)... system.rss:item.view_mode.
This is used when a view sets the view mode to 'default'... see the code in
Drupal\node\Plugin\views\row\Rss
Later on it does
Which is totally weird.
There is very similar code in
Drupal\comment\Plugin\views\row\Rss
I think we have two options here:
Comment #28
alexpottAlso should mention that I discussed with the other committers and we all agreed that at least removing the parts that can never have any effect are "rc target" as a useless UI is very confusing.
Comment #29
alexpottSo here is a patch doing option 1. There is no interdiff because it is a different approach from the previous patches.
If we do option 2 I have no idea about the upgrade path. 'title' is a special view mode hacked into the plugins. The default view mode 'rss' is provided by core but once you change the form you can never re-select. The teaser view mode might exist. The fulltext view mode almost certainly does not. I think we should fix all the view mode wonky-ness in another patch as it is likely to be hard. Basically, the whole view mode setting is currently very broken :(
And the default value is RSS :(
Comment #30
alexpottOpened #2601030: Views RSS view mode settings are completely broken to address the way more complex items.view_mode
Comment #32
alexpottNothing is translatable on the form anymore and this is not the only config that is tested for translatability.
Comment #33
catchMoving to 8.1.x since the form structure changes non-trivially + upgrade path + not sure what happens if an install profile overrides invalid keys.
Comment #34
dawehnerWe should now put some docblock beyond RC
Comment #35
catchAnd the update number should be 8100.
Comment #36
dawehnerLet's add a Novice task for that.
Comment #37
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #38
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented@dawehner Can you please explain what do you mean by
Comment #39
catchThat's for the update defgroup.
Comment #40
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedIt needs reroll tagging to it.
Comment #41
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #42
alvar0hurtad0here's the reroll
Comment #43
alvar0hurtad0:S.... sorry, here's the reroll.
Comment #46
vipul.patil7888 CreditAttribution: vipul.patil7888 as a volunteer and at Faichi Solutions Pvt Ltd commentedComment #47
vipul.patil7888 CreditAttribution: vipul.patil7888 as a volunteer and at Faichi Solutions Pvt Ltd commentedHi,
PFA of patch that will get apply successfully.
Thanks,
Vipul
Comment #48
vipul.patil7888 CreditAttribution: vipul.patil7888 as a volunteer and at Faichi Solutions Pvt Ltd commentedComment #50
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedWhy so ?
80 col ?
base translations ?
Is this necessary ?
Must End with .
Comment #51
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedHow is it ?
Comment #53
mradcliffeGood work so far. I have reviewed the latest patch, #51, with the following notes:
Like @snehi mentioned, these docblocks should contain
@inheritdoc
This is missing a trailing left brace causing the PHP syntax error, which causes the test bot to fail.
This should be updated per @dawhener in comment #34.
What this means is that the dockblock ending at the bottom of the file should be moved above this update and changed to 8.0.0. Then, a new @addtogroup dock block added before and below this update function as 8.1.0.
Still needs to be changed per @catch in #35.
Comment #54
Fred Martin CreditAttribution: Fred Martin as a volunteer commentedComment #56
lokapujyaFixed items in #53 (and rerolled for 8.2. )
Comment #57
xjmThanks @lokapujya.
#56 has a number of out-of-scope coding standards fixes, so marking NW for that.
Comment #58
lokapujyaMaybe we still want those changes in a separate issue.
Comment #60
lokapujyaRedo.
Comment #69
longwaveReviving this as I just noticed the mostly-useless RSS settings form under Web Services and then found #3019793: channel.description is ignored before this issue.
NW as the patch no longer applies, but shouldn't be too hard to reroll.
Comment #70
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedRerolled the patch for 9.1.x. Please review, Thanks.
Comment #71
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #73
longwaveFixed test failures due to parts of #60 being moved around in the codebase over time.
Comment #75
longwaveMissed one.
Comment #77
longwaveCopy/paste error.
Comment #78
quietone CreditAttribution: quietone as a volunteer commentedThis issue summary is out of date, let's get that updated.
Comment #79
SivaprasadC CreditAttribution: SivaprasadC as a volunteer and at Tech Mahindra for Drupal India Association commentedComment #80
SivaprasadC CreditAttribution: SivaprasadC as a volunteer and at Tech Mahindra for Drupal India Association commentedHi,
The patch looks good to me. It applied cleanly to 9.1.x branch. Please find the attached.
+1 RTBC
Comment #81
longwaveThanks for reviewing. Updated IS.
Comment #82
alexpotthttp://codcontrib.hank.vps-private.net/search?text=items.limit&filename= - something is using one of bits of config. At the very least we need an issue created against the module to stop using this... https://www.drupal.org/project/monster_menus - for what it is worth the 8.x-1.x branch has had quite a few recent commits.
The other removals look fine - http://codcontrib.hank.vps-private.net/search?text=system.rss&filename=
Comment #84
longwaveRaised #3178680: Drupal core plans to remove a configuration setting used by this module, back to RTBC.
Comment #85
catchThe monster menus issue is now fixed.
Needs a re-roll, to me this looks fine to land in 9.2.x- there's a tiny chance that some custom code somewhere is doing what monster menus did, but seems very low risk and it's a real bug having these phantom settings in the UI.
Comment #86
SivaprasadC CreditAttribution: SivaprasadC as a volunteer and at Tech Mahindra for Drupal India Association commentedComment #87
SivaprasadC CreditAttribution: SivaprasadC as a volunteer and at Tech Mahindra for Drupal India Association commentedHi @catch,
Based on your suggestion, rerolled the patch. PFA.
Comment #88
longwaveReroll looks good.
Comment #90
catchCommitted cf4a609 and pushed to 9.2.x. Thanks!
Comment #91
longwaveJust noticed that following this the "RSS publishing" description is now wrong:
The first two options no longer exist. Unsure if we need a followup or should just remove this entirely in #2601030: Views RSS view mode settings are completely broken
Comment #92
alex.mazaltovNeed to add @see tag in block comment above function system_post_update_delete_rss_settings()
Comment #93
alex.mazaltovComment #94
alex.mazaltovPlease review and commit
Comment #95
penyaskitoThe landing of this issue on 9.2.x broke my tests on Lingotek module: #3187658: Fix tests depending on system.rss config object as they are failing on 9.2.x
I could just pick any other config object for the purpose of those tests, so don't really matters to me, but wondering if this could affect other modules in different ways, as that wasn't detected on http://codcontrib.hank.vps-private.net search.
I wouldn't expect those to be removed in a minor version anyway, even if unused on core. That's breaking BC.
Comment #96
longwaveSo we have a deprecation policy for config schema (which we broke here, by just removing the schema):
https://www.drupal.org/about/core/policies/core-change-policies/drupal-c...
However, I couldn't find a policy for config entities - but I guess as they have to follow the schema, we should put back the values even if they aren't used, and then only delete them in D10?
Comment #97
longwaveI looked back to see if we had done anything similar and found #3022401: Remove useless config action.settings.recursion_limit
There we also deleted both the config and schema, without deprecations. @alexpott stated in #37
Not saying this is the right thing to do, but we do have precedent for making these sorts of changes in a minor release. However we also didn't follow our own config schema deprecation policy then, either.
Comment #98
catchSo config schema deprecation was introduced for #2829919: Either avoid or explicitly test binary encoding in default configuration (which isn't closed yet, but does have a patch using it). In that case it's used to replace one config key with another.
In the case of this form, we're not changing the structure to a different one, but removing keys that are redundant, so I don't think it's necessary to deprecate first.
However we should probably update the policy to make it clearer what deprecation should be used for.
If we had deprecated then it wouldn't have broken the lingotek module, but that was a test-only breakage (similar to if lingotek was relying to the structure of the RSS settings form itself). I'd feel differently if runtime code broke, but I think it's OK to close this again.
Comment #99
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIn case other modules run into what #95 ran into, let's mention this in the release notes.
Comment #101
penyaskitoI changed my mind and I agree with @catch here in this particular use case. Is quite probable than I would have missed the deprecation warning (if using @legacy, which I don't remember for this particular test) in tests until they actually fail. If I were not using @legacy, the deprecation will break them anyway.
So in the case they are removed and not replaced, breaking builds so people notice ASAP seems like the right thing to do.
Comment #102
mitthukumawat CreditAttribution: mitthukumawat as a volunteer and at Zyxware Technologies for Drupal Association commentedThe feed description and number of items from RSS Publishing setting page seems removed in drupal 9.3.x-dev version. But it still exists in drupal 9.1.
Patch #87 applied cleanly and resolved this.
Comment #103
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCorrect. In order to not disrupt existing sites, this will not get committed to Drupal 9.1. Site owners who are comfortable with adding core patches to their sites are welcome to use #87 until they upgrade to Drupal 9.2.
Comment #104
longwaveNothing left to do here.