Problem/Motivation
Add object that uses the FactoryInterface should have dependencies inject via ::create and not ::__construct(). This allows parent classes to change their constructor with breaking any code.
@see https://events.drupal.org/seattle2019/sessions/drupal-9-coming-getting-y...
Example: \Drupal\config_sync\Controller\ConfigSyncController::create
@see https://www.youtube.com/watch?time_continue=1554&v=hN9KjaBvAUk
@see https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...
Proposed resolution
Fix subclassing and stop overriding constructors.
Remaining tasks
- Search for parent::__construct(
- Move new dependencies to create() and remove __construct()
- FIx broken tests
User interface changes
N/A
API changes
Anyone overriding webform __construct will not be broken but their code should be updated to use the new pattern.
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#110 | 3067546-110.patch | 401.2 KB | jrockowitz |
| |||
#108 | 3067546-107.patch | 401.15 KB | jrockowitz |
| |||
#106 | 3067546-105.patch | 401.15 KB | jrockowitz |
#103 | 3067546-103.patch | 401.15 KB | jrockowitz |
#101 | 3067546-101.patch | 383.17 KB | jrockowitz |
|
Comments
Comment #5
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #7
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #8
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #11
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #12
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedNext steps:
\Drupal
can be change injected dependencies in WebformElement, WebformExporter, and WebformHandler plugins.Comment #17
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #18
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedRemaining tasks
Comment #19
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #21
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #23
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #25
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #26
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedMarking as postponed but the patch is pretty much done and just needs to updated when we merge code into the 3067546-subclassing branch
Comment #30
thallesReplace entity.manager in WebformSubmissionViewController, follow a patch
Comment #31
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedThe patch has to be applicable to the 3067546-subclassing branch with an interdiff.
Comment #32
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedWith this patch, all calls to the 'entity.manager' service have been removed.
Comment #33
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@thalles I ran the deprecated code report on this branch and updated #3042594: Drupal 9 Deprecated Code Report for Webform with all the remaining sub-tasks.
Comment #35
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #37
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #39
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedThe attached patch now includes drupalci.yml, which that will fail tests with deprecation messages.
Comment #42
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedThis patch does not throw errors on deprecation warnings. I think I need to enable 'errors on deprecation warnings' locally first.
Comment #44
BerdirI think most of these deprecation messages are linked to a few remaining entityManager usages, either as $this->entityManager or $container->get('entity.manager').
Most should be easy to address but some might require additional injections. I could provide a separate patch/issue to address them, but if I need another service injected it might conflict with this. So you could also only do it here.
Comment #46
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedMy last few commits might have addressed these remaining calls the 'entity.manager' service.
The attached patch does not have D9 errors turned on. I want to make sure all the tests are passing first.
Comment #48
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #49
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@Berdir I am pretty sure I can get this branch/patch "D9 worthy". I will ping you if I run into any expected gotchas.
Comment #51
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedThis patch includes
suppress-deprecations: false
in drupalci.yml.Comment #52
BerdirNice! Based on a quick glance, the only remaining tests with deprecation messages are integrations tests, so all these are most likely coming from those other modules.
Comment #53
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedWow. I have been maintaining this patch for 6 months and it does seem that this solution/approach worked.
For now, the Webform module is as ready for D9 as it can be.
Comment #54
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedTo help get the Webform module ready for D9, I created #3110478: [Webform 8.x-6.x] Track the D9 readiness state of the Webform module's (optional) dependencies.
Comment #55
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #56
BerdirSomething with that last reroll patch didn't go as it should have :)
Here's a new reroll, conflicted on:
Mostly use statements, the change in WebformGroupManager is gone as that was already updated from what I saw.
Comment #57
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@Berdir Thanks for rerolling the patch. There is the 3067546-subclassing branch to work from. Could you please generate an interdiff that I can apply to that branch?
Comment #59
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@Berdir I am going to do a reroll via the branch.
Comment #60
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #61
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedArg!!! The attached deprecated code report is missing a bunch of things discovered by the testbot.
Comment #63
BerdirDidn't know about the branch, sorry. Found another small thing as well, One class uses the old path.alias_manager instead of path_alias.manager.
This is on top of your patch in #60, not sure why my complete patch file is smaller.
Comment #66
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedTo help move us forward, I bumped the testbot to PHP 7.2 & MySQL 5.5, D8.8
Comment #67
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedI would like to address the deprecated notices via a new ticket which can be committed to Webform 8.x-5.x.
I think the best approach is to fix as much as possible via #3119788: Update deprecated code and then merge that patch into 8.x-5.x and this ticket. We might have to do this pattern a few more times to get this patch/branch 100% D9 ready.
Comment #68
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #69
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #70
DamienMcKenna@jrockowitz: FYI your last patch was empty.
Comment #71
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedThanks for catching that.
Comment #73
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #75
douggreen CreditAttribution: douggreen as a volunteer commentedreroll
Comment #76
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@douggreen Because this is a massive patch and I am tracking the changes via 3067546-subclassing branch, can you please provide an interdiff .
Comment #77
douggreen CreditAttribution: douggreen as a volunteer commentedYes this is a big patch, and I think those diffs are a bit wrong. Can you please review and commit #3133945: Remove .orig files accidentally committed first, these files seem to be messing with my diff creation (not sure why, just that they are). After those are removed, I'll take another stab at a preroll and include the interdiff.
Comment #78
douggreen CreditAttribution: douggreen as a volunteer commentedThis is meant to just be a clean reroll. I don't understand why src/Plugin/WebformSourceEntityBase.php is in the interdiff; the file was created when I applied the patch from #73, so I added it here.
Comment #79
douggreen CreditAttribution: douggreen as a volunteer commentedNote that I still get warnings when running this on D9. These are Symfony changes that will happen with 9.x. Can we fix those here, or do we need to wait for 9.x release?
Comment #80
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@douggreen I am not following changes are being made in #78. I think this patch is as done as it can be and I trying to fix other D9 issues using dedicated tickets and patches.
For #79 checkout #3133255: [Webform 8.x-5.x & 8.x-6.x] WebformExceptionHtmlSubscriber::onException should be compatible with HttpExceptionSubscriberBase::onException
Below are the composer patches, I am using to get a D9 compatible version of the Webform module which still requires some more testing.
Comment #81
douggreen CreditAttribution: douggreen as a volunteer commented> @douggreen I am not following changes are being made in #78.
#73 did not apply to 8.x-5.x-dev, #78 simply is a reroll that should apply.
But I like the idea of the smaller individual patches, I will try them tomorrow.
Comment #82
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedThis is a HUGE patch. I think the best way to move forward is to let me be the sole contributor to this patch via the
3067546-subclassing
branch and all other issues should be addressed via new tickets.Comment #84
douggreen CreditAttribution: douggreen as a volunteer commented#82 adds .orig files again.- user error, sorry, some of the files don't apply cleanly, but do apply, so I get .orig files locallyComment #85
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedFor now, I am going to stop checkout deprecated code, since this can be handled in a new ticket.
Comment #86
neclimdulI've been trying to track down these. Please use your constructors. This is _not_ how this interface is suppose to work and is a broken pattern.
Comment #87
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@neclimdul I do not understand why you changed the status to 'needs work'.
This patch addresses the reality that constructors can change which then breaks any code that extends the constructor.
@see https://www.drupal.org/node/3076421
Here is a recent discussion on Reddit.
Comment #88
neclimdulUgh, I'll follow up there. I'm trying to capture all these and push back. saying "the interface doesn't change" is not true. You're may not _explicitly_ be relying on the constructor's signature but you are implicitly relying on it. if you extend the object your are tied up in the constructor logic and there's no getting away from it.
Comment #89
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #90
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedFor 13 followers of this ticket, the
3067546-subclassing
branch is now somewhat working in Drupal 9.x. It is very likely the3067546-subclassing
will become the Webform 6.x.x working branch.Comment #91
aspilicious CreditAttribution: aspilicious commentedPatch works but it needs a reroll after recent changes.
Comment #92
aspilicious CreditAttribution: aspilicious commentedComment #93
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedPlease provide an interdiff, so that I can commit the changes to
3067546-subclassing
branchI appreciate the help but as stated in #82 I think I should maintain this branch.
The attached reroll patch is
3067546-subclassing
with the latest 8.x-5.x branch merge in.Comment #94
Berdir> Please provide an interdiff, so that I can commit the changes to 3067546-subclassing branch
It's not possible to provide a meaningful interdiff that would actually help you for a rebased patch. These rebases usually happen to apply it to projects, just ignore them and do your thing with the branch.
Just wondering, what's your timeline with the 6.0.x branch, when you do plan to start that?
Comment #95
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedLOL!!! Right now, I am kind of being blocked by a one-line patch for #3137310: Add core_version_requirement: ^8 || ^9 to jquery_ui_tabs.info.yml. I just emailed the maintainer this morning.
I am hoping to tag a 6.0.x alpha by the end of the month. I won't be able to tag a beta until #3110478: [Webform 8.x-6.x] Track the D9 readiness state of the Webform module's (optional) dependencies and I merge in all the pending 6.0.x patches.
Comment #97
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #99
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #100
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #101
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #103
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #106
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #108
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #110
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #112
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #113
thallesCongratulations @jrockowitz!
Comment #114
DamienMcKenna*clap*
Fantastic work, everyone!