Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #N
Problem/Motivation
There are a lot of instances of code like the following in core:
if (is_string($form_arg) && class_exists($form_arg)) {
if (in_array('Drupal\Core\DependencyInjection\ContainerInjectionInterface', class_implements($form_arg))) {
$form_arg = $form_arg::create(\Drupal::getContainer());
}
else {
$form_arg = new $form_arg();
}
}
Proposed resolution
Introduce some generic way to do exactly these kind of code.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#116 | interdiff-2165475-116.txt | 639 bytes | damiankloip |
#116 | 2165475-116.patch | 35.13 KB | damiankloip |
#81 | 2165475-81.patch | 744.11 KB | damiankloip |
#75 | interdiff.txt | 814 bytes | dawehner |
#75 | class_resolver-2165475-75.patch | 34.26 KB | dawehner |
Comments
Comment #1
dawehnerThis is for example useful one some page manager functionality.
Comment #2
tim.plunkettI like it!
I rounded out the usage to show it off better.
Comment #4
dawehnerFixed the bit in the installer.
Comment #5
dawehnerMissing interdiff
Comment #6
dawehnerRerolled with a nice suggestion from chx!
Comment #7
tim.plunkettNice change!
No interdiff, but the change was swapping
if (in_array('Drupal\Core\DependencyInjection\ContainerInjectionInterface', class_implements($class))) {
for
if (is_subclass_of($class, 'Drupal\Core\DependencyInjection\ContainerInjectionInterface')) {
Comment #8
sunShouldn't this be a static method?
It doesn't appear to use or touch the state of the instantiated class in any way, it's just an I/O utility operation?
Injecting an entire ControllerResolver instance just to be able to figure out how to instantiate certain classes based on a hard-coded pattern is a completely unnecessary dependency?
Am I missing something big here?
Comment #9
tim.plunkettA completely different approach, via @msonnabaum.
Comment #10
dawehnerNot sure what the advantages of static methods would be.
Comment #12
Crell CreditAttribution: Crell commentedHonestly there aren't any. Objects aren't something you use only when you have state. They're something you use to provide a mostly-free layer of indirection, or to encapsulate functionality.
And this should still be done upstream...
Comment #13
dawehnerWell, the controller resolver has state, maybe this is just a sign that this method does not belong into this class.
Comment #14
dawehnerWe also have the status of the container.
Comment #16
dawehnerRerolled.
Comment #18
tim.plunkettFixed the unit test.
Comment #19
sunSomething still feels wrong with this approach. Perhaps my interpretation is wrong, but let me try to think aloud (sorry in advance if I'm not using correct/proper terminologies):
All of this sounds very concerning to me. Please let me know where I'm wrong.
Comment #20
tim.plunkettResponding to 19.4 for now, I'll be back for the rest
Umm. HtmlFormController and EntityManager already had $this->container. The others called \Drupal::getContainer(). This is giving them *less* surface area, not more.
Comment #21
twistor CreditAttribution: twistor commentedThis comment?
I agree that ControllerResolver is not an ideal place to stick this bit of functionality, however, anything creating ContainerInjectionInterface objects has an obvious dependency on the container.
Comment #23
dawehnerThis patch started to move everything into a class_resolver so we get rid of controller problem.
Comment #24
cordoval CreditAttribution: cordoval commentedis there a github mirror PR when we can see the diff more beautifully? would help a lot
Comment #25
dawehnerRerolled the patch.
Comment #27
martin107 CreditAttribution: martin107 commentedCorrected mismatch in arguments between constructor definition and arguments supplied to controller_resolver service definition.
Comment #29
martin107 CreditAttribution: martin107 commentedclass_resolver :-
service definition argument... a service container ... fair enough.
class_resolver constructor missing ... opps container gets dropped on floor.
Fixed.
Comment #30
dawehnerGood catch! On top of that it would be great to write some test coverage.
Note: we can use the ContainerAwareTrait from Symfony\Component\DependencyInjection instead.
Opened a new issue for other instances: #2229183: use ContainerAwareTrait instead of extending ContainerAware
Let's use setter injection instead so we do something similar as all the other places in core.
Comment #32
martin107 CreditAttribution: martin107 commentedNo Need to use ContinaerAwareTraits... as we are extending ContainerAware..
In this case less is more... So I have backed out the constructor and altered the service definition.
Facepalm Adding traits -- results in duplication of functionality errors .. I wish I had seen that before I did it
Comment #33
martin107 CreditAttribution: martin107 commentedComment #36
martin107 CreditAttribution: martin107 commentedThis change addresses one flaw... its a simple change but requires lots of changes to variables up and down the lasagne.
This needs careful review as I wanted to find a better solution but could not
HtmlFormController.php requires both a
$classResolver ...see getFormObject()
AND
$controllerResolver... as it calls its parent::__contruct on FormController
IT was previous fed only a ControllerResolver
NB HtmlFormController:getFormObject(Request $request, $form_arg) --- $request is an unused variable.. something wrong there...
Comment #38
dawehnerNote, we don't use camelCased variables as method parameters.
Comment #39
martin107 CreditAttribution: martin107 commentedThree things
1) Single line reroll from core.services.yml due to class with cache.default being renamed. The interdiff.txt is from this point onwards
2) Arguement parameters no longer camel cased. ( Thank you for pointing that out )
3) Next issue on the hit list...from core/tests/Drupal/Tests/Core/Form/FormTestBase.php
Previously controllerResolver was supplied as parameter, when classResolver was needed.
These recent patches are slowly correcting issues... the tesbot error logs are getting shorter. BUT Further failures are expected.
Comment #40
martin107 CreditAttribution: martin107 commentedComment #42
martin107 CreditAttribution: martin107 commentedThis controller/class resolver bug is common to many tests.
Comment #43
martin107 CreditAttribution: martin107 commentedComment #45
dawehnerFixed the failures.
Comment #46
jibranSo another DIY. If my doc changes are fine then this patch is RTBC.
\ Not needed
@param @throw and @return missing.
more then 80 chars.
Comment #47
tim.plunkettThere are two spaces here.
Why both?
We removed this a looooong time ago
Missing blank line
This needs to explain when it is thrown
Please swap the order of these two
Comment #48
martin107 CreditAttribution: martin107 commentedassigning to myself while I resolve #47
Comment #49
martin107 CreditAttribution: martin107 commentedAll issues fixed....
I hope you don't consider this feature creep but in fixing #47.6,
once I swapped the type hinting to reflect the coding standard I have to make the file look consistent and so I had to also swap 10 others
Comment #50
jibranThanks @martin107 for fixing the issues. I have only made some doc changes so I think I can RTBC this patch.
Comment #51
dawehnerAwesome, thank you for pushing that forward!
Comment #52
damiankloip CreditAttribution: damiankloip commentedPatch looks great, just did a full review and test.
I'm not sure how much getClassResolverStub() will actually be used, but makes sense to keep it I think.
+1 - Just added one missing space as I had the patch applied!
Comment #53
dawehnerMost trivial interdiff of the month :P
Comment #54
damiankloip CreditAttribution: damiankloip commentedIt's all about the finer details :D
Comment #55
damiankloip CreditAttribution: damiankloip commentedComment #57
martin107 CreditAttribution: martin107 commentedReroll.
Comment #58
martin107 CreditAttribution: martin107 commentedTrivial reroll.... so back to rtbc
Comment #59
damiankloip CreditAttribution: damiankloip commentedYep, looks good to me.
Comment #61
martin107 CreditAttribution: martin107 commentedI will do the reroll
Comment #62
martin107 CreditAttribution: martin107 commentedLots of niggly changes any of which could blow up testing
so manual testing :-
1) It installs :-)
2) ControllerResolverTest passes
3) EntityManagerTest passes
Lets see what test bot says
Comment #63
martin107 CreditAttribution: martin107 commentedGreen.
Comment #64
dawehnerBack to RTBC
Comment #65
alexpott62: class_resolver-2165475-62.patch queued for re-testing.
Comment #66
xjmI think this is significant enough to maybe merit a change record for the addition?
Also, here's a reroll without the trailing whitespace. ;)
Comment #67
dawehnerSure, why not: https://drupal.org/node/2245853
Comment #69
Jalandhar CreditAttribution: Jalandhar commentedUpdating rerolled patch.
Comment #70
martin107 CreditAttribution: martin107 commentedPatch looks good so back to RTBC
Comment #71
dawehnerThank you for rerolling!
Comment #72
catchShouldn't the method be getInstanceFromClassOrServiceName()?
getInstanceFromClassName() does class_exists() and also handles services, so can't this just use the method? If the logic is different it could use a comment as to what needs changing.
Or service name right? Docs could be much clearer about this accepting both.
Also:
This looks like it could be a performance regression. When you request a service that doesn't exist, Symfony runs a levenshtein comparison on the string against all services to create suggestions as part of the exception it throws for missing services.
I didn't check if that's the case here, but it might work out cheaper to do the class_exists() check first. At least, this needs checking.
Comment #73
dawehnerWent with getInstanceFromDefinition which is more similar with the controller resolver.
Doesn't the has call, which is pretty straightforward:
ensures that you never call a non-existing service?
Comment #75
dawehnermeh
Comment #76
tim.plunkettThis needs a reroll after #2209977: Move form validation logic out of FormBuilder into a new class, and will need another one after #2257835: Move form submission logic out of FormBuilder into a new class which is RTBC, so I'm just going to wait and do that
Comment #77
dawehnerThis one has been RTBC for ages, well.
Comment #78
tim.plunkettOkay, if someone else wants to reroll it before then, feel free. I just know that the constructor signature of FormBuilder *just* changed, and is about to change again.
Comment #79
dawehnerYeah, let's waste all the time!
Comment #80
dawehnerLet's make it like the professionals and mark the other issue as being blocked on this one.
Comment #81
damiankloip CreditAttribution: damiankloip commentedHere is a reroll either way. Entity Manager needed it too.
Comment #82
damiankloip CreditAttribution: damiankloip commentedUsually good to rebase that branch too.
Comment #83
damiankloip CreditAttribution: damiankloip commentedFFS!!
Comment #84
damiankloip CreditAttribution: damiankloip commentedMeh. SOrry for inappropriate noise.
Comment #85
dawehnerHA, you went through all the steps!!
Comment #89
damiankloip CreditAttribution: damiankloip commentedThanks whoever did mass cancelling!!
Comment #90
catchLooks like there's still some cruft that could be removed:
Comment no longer applies.
You're right that the has() call protects against needless container lookups, sorry for false alarm.
Do we really still need the class_exists() here? That should be covered.
Again is the class_exists() necessary?
Why is this needed if the class resolver can get a service anyway before it reaches here?
Comment #91
dawehnerGood points.
Comment #92
tim.plunkettAh, nice one.
Thanks @catch!
Comment #94
catch91: class_resolver-2165475-91.patch queued for re-testing.
Comment #96
damiankloip CreditAttribution: damiankloip commented91: class_resolver-2165475-91.patch queued for re-testing.
Comment #98
damiankloip CreditAttribution: damiankloip commentedThat is what was keeping the logic to return the original string for the form ID if just a string is provided. Otherwise, we assume it's a class when it's just a string like 'foo' or something. See FormBuilderTest::testGetFormIdWithString(). So we can add this back like this patch does, or re-think how we deal with this using the ClassResolver.
Comment #99
catchOK so let's keep that as is, however it could use a comment containing the basics of #98.
Comment #100
damiankloip CreditAttribution: damiankloip commented?
Comment #101
catchYeah that's great. Back to RTBC.
Comment #102
damiankloip CreditAttribution: damiankloip commentedSuper.
Comment #104
damiankloip CreditAttribution: damiankloip commentedRerolled. Also, I think we can just remove the container aware stuff for the controller resolver now as the class resolver handles that?
Comment #105
damiankloip CreditAttribution: damiankloip commentedComment #107
damiankloip CreditAttribution: damiankloip commentedSilly.
Comment #108
damiankloip CreditAttribution: damiankloip commentedComment #110
dawehnerMaybe this time ...
Comment #112
damiankloip CreditAttribution: damiankloip commentedAnother reroll of that.
Comment #114
damiankloip CreditAttribution: damiankloip commentedNow I forgot to pull. Here is a re-re-base.
Comment #116
damiankloip CreditAttribution: damiankloip commentedComment #117
damiankloip CreditAttribution: damiankloip commentedComment #118
dawehnermoeh
Comment #119
alexpottCommitted 15cf730 and pushed to 8.x. Thanks!