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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rslootjes created an issue. See original summary.

Munavijayalakshmi’s picture

Assigned: slootjes » Munavijayalakshmi
Status: Active » Needs review
FileSize
717 bytes
dawehner’s picture

+++ b/core/core.api.php
@@ -2518,7 +2518,7 @@ function hook_validation_constraint_alter(array &$definitions) {
- * Events are part of the Symfony framework: they allow for different components
+ * Events are part of the Symfony Http Kernel: they allow for different components

Now we exceed 80 chars :(

Munavijayalakshmi’s picture

@dawehner I changed it. Check it once.

slootjes’s picture

The 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.

slootjes’s picture

New 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?

Status: Needs review » Needs work

The last submitted patch, 6: 2853183-6.patch, failed testing.

gaurav.kapoor’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
abhishek_10’s picture

Status: Needs review » Reviewed & tested by the community

Tested and working.

The last submitted patch, patch.diff, failed testing.

gaurav.kapoor’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/core.api.php
@@ -2518,11 +2518,13 @@ function hook_validation_constraint_alter(array &$definitions) {
+ * Symfony Http Kernel @endlink: they allow for different components

Not 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.

slootjes’s picture

Http 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?

alexpott’s picture

The 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...

himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
1.7 KB

Added the new patch, as per #14 .

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/core.api.php
@@ -2518,14 +2518,15 @@ function hook_validation_constraint_alter(array &$definitions) {
+ * time; many events are dispatched by Drupal core and the Symfony framework in

Drupal 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.

himanshu-dixit’s picture

Changes 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.

slootjes’s picture

I 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.

alexpott’s picture

@slootjes

but I think it's good to show the relation it has to the Http Kernel which relies on this system.

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.

slootjes’s picture

In 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.

Munavijayalakshmi’s picture

Assigned: Munavijayalakshmi » Unassigned
anemes’s picture

Issue tags: +DCTransylvania
slootjes’s picture

"Needs work", what needs to be done exactly? This simple documentation change is open for almost 3 months...

mpdonadio’s picture

Based 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".

prash_98’s picture

Status: Needs work » Needs review
FileSize
1.35 KB

Remove the mention of symfony framework from the core.api.php. Please review the changes.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

Status: Needs review » Needs work
+++ b/core/core.api.php
@@ -711,7 +711,7 @@
- * Drupal from the @link http://symfony.com/ Symfony framework. @endlink A
+ * Drupal from the @link http://symfony.com/doc/current/components/dependency_injection.html. @endlink A

This goes over 80 cols and the comment should be reflowed to be under that.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

shubham.prakash’s picture

Version: 8.6.x-dev » 8.9.x-dev

This issue is still present, will add patch for the same.

shubham.prakash’s picture

Status: Needs work » Needs review
FileSize
2.82 KB

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tanubansal’s picture

After adding #31, its still showing the same text.
Issue still persists

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Title: Correct reference from Symfony Framework to Http Kernel » Correct references from Symfony Framework
Issue summary: View changes
Issue tags: +Bug Smash Initiative
FileSize
3.26 KB
4.94 KB

Came 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.

darvanen’s picture

Status: Needs review » Needs work
  1. +++ b/core/core.api.php
    @@ -734,18 +734,19 @@
    + * Drupal from the
    + * @link http://symfony.com/doc/current/components/dependency_injection.html.
    + * @endlink A "service" (such as accessing the database, sending email, or
    

    Link no longer has any inner text, making the sentence read:
    "... have been adopted by Drupal from the A "service" (such as ..."

  2. +++ b/core/lib/Drupal/Core/Routing/routing.api.php
    @@ -65,7 +65,7 @@
    - *   instructions (it has other uses in the Symfony framework). Most
    + *   instructions (it has other uses in the Symfony components). Most
    

    Nit: shouldn't this be "other uses in Symfony components" rather than "other uses in the Symfony components"?

    I.e. remove "the".

quietone’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
5.29 KB

@darvanen, thank you.

Attached patch should fix #37 1 and 2.

darvanen’s picture

Status: Needs review » Needs work

Thanks @quietone, and sorry but:

+++ b/core/core.api.php
@@ -734,18 +734,19 @@
  * The Services and Dependency Injection Container concepts have been adopted by
...
+ * Drupal from the
+ * @link http://symfony.com/doc/current/components/dependency_injection.html. @endlink
+ * A "service" (such as accessing the database, sending email, or
+ * translating user interface text) is defined (given a name and an interface or

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.

quietone’s picture

Status: Needs work » Needs review
FileSize
735 bytes
5.33 KB

Oh 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 :-)

JohnAlbin’s picture

Status: Needs review » Needs work
+ * @link http://symfony.com/doc/current/components/dependency_injection.html. Symfony DependencyInjection Component @endlink

Sorry, 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!

darvanen’s picture

Status: Needs work » Needs review
FileSize
5.33 KB
2.22 KB

Addressed #41

JohnAlbin’s picture

Status: Needs review » Needs work

Oops. I missed this in the previous review. Sorry about that!

+ * dispatches the event at an  appropriate time; many events are dispatched by

There's a double space in the middle of this sentence.

darvanen’s picture

Status: Needs work » Needs review
FileSize
5.33 KB
725 bytes

Addressed #43.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

All the words have bee replaced and all the nits have been picked. Can't find any new ones either. Looks good!

quietone’s picture

Thanks everyone for finishing this one! I, obviously was all thumbs, but then it was near 5pm on Friday when I made those patches.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/core.api.php
@@ -2496,14 +2497,13 @@ function hook_validation_constraint_alter(array &$definitions) {
- * concept of events, see
+ * Events allow different components of the system to interact and communicate
+ * with each other. Each event has a unique string name. One system component

This 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...

darvanen’s picture

Yes that's true, what do we think of this?

 * with each other. Each event has an event class (or a unique string name -
 * deprecated as of Symfony 4). One system component dispatches the event at an
catch’s picture

The unique string name isn't deprecated as of Symfony 4, they just swapped the arguments to make it optional.

darvanen’s picture

If 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.

quietone’s picture

Since 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?

darvanen’s picture

Status: Needs review » Reviewed & tested by the community

Certainly the simplest approach, I have no problem with it.

Have double-checked for regressions, going to put this back to RTBC.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: 2853183-51.patch, failed testing. View results

Spokje’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC per #52 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

  • catch committed 462b3b7 on 10.0.x
    Issue #2853183 by quietone, darvanen, slootjes, Munavijayalakshmi,...
  • catch committed 2ea9f64 on 9.4.x
    Issue #2853183 by quietone, darvanen, slootjes, Munavijayalakshmi,...

  • catch committed f15f24a on 9.3.x
    Issue #2853183 by quietone, darvanen, slootjes, Munavijayalakshmi,...
catch’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x and cherry-picked to 9.4.x and 9.3.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.