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.
Problem/Motivation
In the core.api.php this is mentioned: "Events are part of the Symfony framework". While this on it's own is true, I think it would be better to change it to "Events are part of the Symfony Http Kernel" instead as Drupal is not using the Framework but rather the components and it is less confusing.
Steps to reproduce
Proposed resolution
Use phrases such as "Symfony routing system" and "Symfony event system" and "Symfony components" instead of Symfony Framewoek.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#51 | 2853183-51.patch | 5.29 KB | quietone |
#51 | interdiff-44-51.txt | 1.31 KB | quietone |
#44 | 2853183-44.patch | 5.33 KB | darvanen |
#36 | interdiff.txt | 3.26 KB | quietone |
#33 | Screen Shot 2020-07-30 at 2.42.26 PM.png | 479.57 KB | tanubansal |
Comments
Comment #2
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedComment #3
dawehnerNow we exceed 80 chars :(
Comment #4
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commented@dawehner I changed it. Check it once.
Comment #5
slootjes CreditAttribution: slootjes commentedThe second reference wasn't correctly updated, it still mentions Symfony framework now.
"many events are dispatched by Drupal core and the Symfony framework in every request." should become "many events are dispatched by Drupal core and the Symfony HttpKernel in every request.".
Also, the link to the HttpKernel component is missing.
Comment #6
slootjes CreditAttribution: slootjes commentedNew patch which preserves the link and second reference to Http Kernel and also stays under 80 characters.
edit: No idea why it's failing, can someone explain me?
Comment #8
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #9
abhishek_10 CreditAttribution: abhishek_10 at OpenSense Labs commentedTested and working.
Comment #11
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #12
alexpottNot sure Http Kernel is the right thing here. We use the event component. Events are not just part of the Http Kernel - which in of itself is a component that leverages the event component to allow other components to interact with it.
Comment #13
slootjes CreditAttribution: slootjes commentedHttp Kernel consists of the Event Dispatcher and Http Foundation component so it also covers the events. I think it is more suiting that calling this the "Symfony framework" as this is a completely different thing. Do you have a better suggestion perhaps?
Comment #14
alexpottThe problem is we have events that have nothing to do with the Http Kernel component. For example:
\Drupal\Core\Config\ConfigEvents
and many others. Why not just say we use the "Symfony EventDispatcher component to allow your application components to communicate with each other by dispatching events and listening to them"I.e. borrowing language from https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Eve...
Comment #15
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commentedAdded the new patch, as per #14 .
Comment #16
alexpottDrupal core and Symfony components
We also should the usages of "Symfony Framework" if it is wrong - the only other two are in routing.api.php.
Comment #17
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commentedChanges in routing.api.php should be done after this is fixed, #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing , Currently that is a major issue.
Comment #18
slootjes CreditAttribution: slootjes commentedI think the confusion is in people calling it "the Symfony framework". Drupal 8 is not using the framework, it's using some Symfony components. Referring to the Event Dispatcher component could also be fine (at least better than it is now) but I think it's good to show the relation it has to the Http Kernel which relies on this system.
I also looked at the other mention in core.api.php "The Services and Dependency Injection Container concepts have been adopted by Drupal from the Symfony framework." and I feel this is actually true as it's talking about a concept from the framework specifically which isn't dictated by the components while Http Kernel does dictate to use events because otherwise it can not function.
Comment #19
alexpott@slootjes
But so much more uses the event system. Other parts of the system rely on events too and we don't need to list them all here.
Comment #20
slootjes CreditAttribution: slootjes commentedIn my opinion the core.api.php docs are about the internals and the main use of having this Event Dispatcher is about using the Http Kernel which can not function without it. The rest was added on top of that because the system was already there. I do agree it would probably be better to replace it with Event Dispatcher but maybe it's good to add a reference to Http Kernel as it's the base of how requests are handled.
Comment #21
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedComment #22
anemes CreditAttribution: anemes at PitechPlus commentedComment #23
slootjes CreditAttribution: slootjes commented"Needs work", what needs to be done exactly? This simple documentation change is open for almost 3 months...
Comment #24
mpdonadioBased on the comments in #16 and IRC w/ @dawehner we need to update the remaining reference to "Symfony framework" to "Symfony event system" (or similar) in core.api.php. Then we need to update routing.api.php update the references to "Symfony framework". I think the first can be "Symfony routing system" and the second can me "Symfony components".
Comment #25
prash_98 CreditAttribution: prash_98 at Google Summer of Code commentedRemove the mention of symfony framework from the core.api.php. Please review the changes.
Comment #28
borisson_This goes over 80 cols and the comment should be reflowed to be under that.
Comment #30
shubham.prakash CreditAttribution: shubham.prakash at OpenSense Labs commentedThis issue is still present, will add patch for the same.
Comment #31
shubham.prakash CreditAttribution: shubham.prakash at OpenSense Labs commentedComment #33
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedAfter adding #31, its still showing the same text.
Issue still persists
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedCame to triage for BugSmash and ended up making the corrections suggested in #24. And then changed the last last reference to 'Symfony framework' in core.api.php.
Uploading patch with interdiff and not running tests because this is doc only.
Comment #37
darvanenLink no longer has any inner text, making the sentence read:
"... have been adopted by Drupal from the A "service" (such as ..."
Nit: shouldn't this be "other uses in Symfony components" rather than "other uses in the Symfony components"?
I.e. remove "the".
Comment #38
quietone CreditAttribution: quietone as a volunteer commented@darvanen, thank you.
Attached patch should fix #37 1 and 2.
Comment #39
darvanenThanks @quietone, and sorry but:
This link still has no text in it. There needs to be something between the URL and @endlink. Also it now goes over 80 chars.
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedOh my. Thanks for your patience! I finally see the problem.
I added the text Symfony DependencyInjection Component. I checked the line length earlier with, @link: HTML links, and it can be > 80 chars.
Crossing my fingers :-)
Comment #41
JohnAlbinSorry, but you've incorrectly added a period to the end of the URL. And it looks like the period is needed at the end of the sentence.
Based on line 1440 of core.api.php and the output on its Drupal API web page, @link lines can wrap multiple lines and don't need to be contained on one line. So please re-wrap that paragraph and its @link so it is less than 80 characters per line.
I searched the Drupal codebase (
Symfony\s+\*?\s*[Ff]
) and couldn't find any other mention of Symfony framework. So, everything else looks good!Comment #42
darvanenAddressed #41
Comment #43
JohnAlbinOops. I missed this in the previous review. Sorry about that!
There's a double space in the middle of this sentence.
Comment #44
darvanenAddressed #43.
Comment #45
borisson_All the words have bee replaced and all the nits have been picked. Can't find any new ones either. Looks good!
Comment #46
quietone CreditAttribution: quietone as a volunteer commentedThanks everyone for finishing this one! I, obviously was all thumbs, but then it was near 5pm on Friday when I made those patches.
Comment #47
catchThis is no longer true, it's either a string name or an event class.
Since Symfony 4, but backported to Drupal 9, EventDispatcher::dispatch() takes an event class as the first argument, and the name argument is no longer required.
https://api.drupal.org/api/drupal/vendor%21symfony%21event-dispatcher%21...
Comment #48
darvanenYes that's true, what do we think of this?
Comment #49
catchThe unique string name isn't deprecated as of Symfony 4, they just swapped the arguments to make it optional.
Comment #50
darvanenIf you're talking about having one or the other it's technically deprecated, but I take your point.
I'm usually good with words but my brain seems to be refusing to come up with a succinct alternative right now.
Comment #51
quietone CreditAttribution: quietone at PreviousNext commentedSince this is an overview of event dispatching it should be fineto leave out the detail about how the Event is uniquely identified. This removes the sentence, 'Each event has a unique string name.'.
What do you think?
Comment #52
darvanenCertainly the simplest approach, I have no problem with it.
Have double-checked for regressions, going to put this back to RTBC.
Comment #55
SpokjeBack to RTBC per #52 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.
Comment #58
catchCommitted/pushed to 10.0.x and cherry-picked to 9.4.x and 9.3.x, thanks!