Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
drupal-check results on commit hash:
source : [git] https://git.drupal.org/project/workbench_moderation 661c2108c84e961a00ae7174c91053858447a7d2
source : http://cgit.drupalcode.org/workbench_moderation
Fatal error: Declaration of Drupal\workbench_moderation\Plugin\Menu\EditTab::getTitle() must be compatible with Drupal\Core\Menu\LocalTaskDefault::getTitle(?Symfony\Component\HttpFoundation\Request $request = NULL) in /Users/dwaynemcdaniel/Documents/d9-contrib/midcamp2019-drupal/web/modules/contrib/workbench_moderation/src/Plugin/Menu/EditTab.php on line 16
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#30 | 3042757-30.patch | 65.56 KB | larowlan |
#30 | 3042757-30-interdiff.txt | 34.92 KB | larowlan |
#29 | 3042757-29.patch | 30.94 KB | larowlan |
#29 | 3042757-29-interdiff.txt | 2.57 KB | larowlan |
#28 | 3042757-28.patch | 28.37 KB | larowlan |
Comments
Comment #2
larowlanComment #3
joy29 CreditAttribution: joy29 commentedComment #4
joy29 CreditAttribution: joy29 commentedComment #5
joy29 CreditAttribution: joy29 commentedComment #6
tatarbjComment #7
stevectorI ran the checker locally and got many more results than were reported here: https://github.com/mcdwayne/Drupal-9-drupal-check-Report/blob/master/Dru...
joy29, did you run the drupal-check tool locally?
Comment #8
larowlanComment #9
agentrickardI wonder...can we just drop this module in Drupal 9? Do we have a clean upgrade path to core Content Moderation?
Comment #10
redzeufGood point @agentrickard, I guess this module should be a minimal maintain for D9 just because of all the D8 websites that used it already and don't have the capacity to migrate to the core Content Moderation.
Second point, we should update the scope in the summary of this issue regarding the comment #7 #3042757-7: Drupal 9 Deprecated Code Report
Comment #11
larowlanAs much as I would *love* to just not update WBM for Drupal 9, I think that presents a problem.
As a community, our positioning and narrative around Drupal 8 to 9 has been 'Drupal 9 will be a painless update'.
And that is the case if the modules you have on Drupal 8 are updated for Drupal 9 between now and June 2020.
The issue is, there are still more than 5000 D8 sites using WBM (see https://www.drupal.org/project/usage/workbench_moderation), and its most likely that nearly all of these are early adopters of D8.
Having been through the WBM » CM migration on some production sites, I can say from experience that it is not 'painless'. So if we say 'there will be no D9 compatible WBM release' it will be contradictory to the 'Drupal 9 will be a painless update' narrative we've been telling as a community.
So I think for that reason we should provide a D9 version, but we should then put the module into a 'no further development' state and aggressively mark feature requests and non-critical bugs as Closed (Won't fix)
I intend to try and work on this at DrupalSouth 2019 code sprint (later this month).
Comment #12
agentrickardWhat would we need to do to make the transition 'painless'? Do we have a list?
Comment #13
Tolyan4ik CreditAttribution: Tolyan4ik at EPAM Systems commentedPlease check
Comment #14
mattbloomfield CreditAttribution: mattbloomfield as a volunteer commentedHi,
I've applied the patch in #13, but I'm trying to install on an instance of Drupal 9. I'm getting the following error in my php log:
PHP Exception InvalidArgumentException: "No converter has been registered for paramconverter.latest_revision" at /srv/bindings/991c5136570c4bcfab407f525abcd258/code/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php line 39
And in drush I get this error:
You have requested a non-existent service "entity.manager".
It does say entity_type.manager in the services file, so I don't know where the error could be.
Comment #15
larowlanComment #16
larowlanComment #17
larowlanPostponed on #3121549: Allow installation in D9
Comment #18
agentrickardI *really* don't think we should allow new installations in Drupal 9. Force new users to use Content Moderation.
Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedBlocker is in.
Comment #20
larowlanComment #21
larowlanComment #22
larowlanComment #23
larowlanComment #25
larowlanMissed a comma in the annotation change
Comment #26
larowlanComment #27
larowlanComment #28
larowlanComment #29
larowlanComment #30
larowlanComment #31
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi @larowlan
The latest patch returns with 317 coding standards messages. See: https://www.drupal.org/pift-ci-job/1719773
Comment #32
larowlanThanks @munish.kumar - but it also notes
"✓ 10 fewer than branch result"
So we're getting better.
I'd prefer to keep this focussed on D9 functionality and open a separate issue to fix all of those once this is in.
Comment #33
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@larowlan, Thanks for the clarification. I agree with your thoughts, It would also help people to update to D9.
Comment #34
larowlanDiscussed this with @munish.kumar via slack, and he agreed with the above. Crediting him for his time to reply.
I opened #3150430: Fix coding standards for that.
Comment #35
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedLGTM, nothing worth blocking over, especially given the module is mostly on life support.
Reference CM not WBM, but hardly worth fixing.
Supporting syncing and EntityPublishedInterface seem like new features, which is interesting to add this late in the game, although hopefully not disruptive. Seems like it would be fine IMO, just pointing it out.
I don't really understand the implications of removing this check. I'm reading it as: WBM never supported moderating translations independently but now you can?
Multilingual and pending revisions is very tricky, if it's purely as a new feature, it might be worth just leaving the restriction in there and directing people to upgrade to CM if they need translations.
At least then, the bugs will be in one queue.
Comment #36
larowlanThanks @Sam152
1-3 were to get existing ModerationLocaleTests tests passing on D9, not trying to adding new features. I followed the lineage on content moderation in terms of what happened to the code that was in NodeModerationHandler, and the commits that changed - we already supported multilingual, but without those changes it was failing on D9. So something changed in either NodeForm or similar - so this is basically a backport from CM
Comment #38
larowlanComment #39
penyaskito@larowlan
This raises the requirement from PHP 7.0 to 7.1. Not sure if that was in purpose given that I assume this will still be low maintenance.
https://3v4l.org/aieRN
Comment #40
larowlan@penyaskito argh yeah that's PHPStorm it auto changes that with my current language level settings.
Thoughts - 7.0 is long EOL and based on https://www.drupal.org/docs/system-requirements/php-requirements not recommended anymore.
I'm happy to wind it back if needs be, but realistically people need to be moving towards 7.3 at this point in time.
Comment #42
joshua.boltz CreditAttribution: joshua.boltz at Mediacurrent commentedI don't understand why this was marked as Fixed, because I've upgraded to the latest version of this module (which includes this patch to add D9 fixes), but when i re-run the Drupal-check, i still see these errors
So not sure how these were missed as part of this issue.
Comment #43
larowlan@joshua.boltz other than the incorrect test namespace for the views test, none of those files exist in workbench moderation HEAD.
So I guess that means you've got a patch applied? That patch will need updating for D9 would be my guess.
I'll open an issue for the test namespace - #3170945: Class ViewsDataTest does not match filename ViewsDataIntegrationTest.php