Over in #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration, we got derailed simply trying to introduce a new Bundle class "EditorBundle", which extended the "Bundle" class. Following the standard pattern used throughout core, this class's docblock was given the description "Editor dependency injection container". However @tstoeckler pointed out that this class isn't a dependency injection container. It simply registers a service with the Drupal dependency injection container.

A search on our current code base reveals this documentation is routinely copy/pasted into new implementations of the Bundle class, all of which are inaccurate. Here's a count on each of the 21 Bundle classes we currently have in the core/modules directory:

5: Undocumented (FieldSqlStorageBundle, FileBundle, TestBundle, UrlAlterTestBundle)
1: "X bundle class" (BundleTestBundle)
1: "Defines the X Bundle" (BanBundle)
1: Actually describes something: "Registers a dynamic route provider." (RouterTestBundle)
13: "X dependency injection container."

We should figure out what description is accurate to describe these bundles and update our implementations throughout core.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

My suggestion over there was simply

Bundle for Foo module.

The problem with that is that is violates the "Start with a third-person verb"-rule. I could not for the life of me, though, figure out a meaningful way to prepend that sentence with a verb. We could of course do something like

Defines the bundle for Foo module.

Wim Leers’s picture

What about:

Registers service definitions for the X module.

Pretty much every bundle's build() method does only call $container->register(). That method has the following docblock:

    /**
     * Registers a service definition.
     *
     * This methods allows for simple registration of service definition
     * with a fluid interface.
     *
     * @param string $id    The service identifier
     * @param string $class The service class
     *
     * @return Definition A Definition instance
     *
     * @api
     */
    public function register($id, $class = null)

So that's where I get my proposal from.

quicksketch’s picture

Registers service definitions for the X module.

+1

This actually describes what the class does, rather than rephrasing the class declaration. If your bundle class does *not* register services, obviously this would be replaced with something else. I think this will help avoid patterns and encourage actual documentation that describes what's happening in the bundle class.

One more question though, what are "service definitions"? Would "services" be sufficient? "Registers services for the X module."

Wim Leers’s picture

#3: your additional question is a great one. I wondered the same thing :) Apparently Symfony's ContainerBuilder distinguishes between the service identifier and the service definition. This makes sense, but I wonder if we can treat that as an implementation detail and omit the "definitions" part from our bundle docblocks. My intuition says "yes", but I don't have a strong opinion about this.

sun’s picture

Just "Registers services for the X module." or even shorter, "Registers X module services." is totally correct and fine and the way to go.

Wim Leers’s picture

Alrighty, let's do it :)

effulgentsia’s picture

Status: Active » Needs review
FileSize
12.27 KB
effulgentsia’s picture

FileSize
11.05 KB
diff --git a/core/modules/edit/tests/modules/lib/Drupal/edit_test/Plugin/edit/processed_text_editor/TestProcessedEditor.php b/core/modules/edit/tests/modules/lib/Drupal/edit_test/Plugin/edit/processed_text_editor/TestProcessedEditor.php
deleted file mode 100644
--- a/core/modules/edit/tests/modules/lib/Drupal/edit_test/Plugin/edit/processed_text_editor/TestProcessedEditor.php
+++ /dev/null

Oops. Rogue hunk from another issue. Cleaned up in this one.

quicksketch’s picture

In a few parts of effulgentsia's patch, we're deleting useful documentation:

- * Bundle class for mandatory core services.
- *
- * This is where Drupal core registers all of its services to the Dependency
- * Injection Container. Modules wishing to register services to the container
- * should extend Symfony's Bundle class directly, not this class.
+ * Registers core Drupal services.
 /**
- * Test bundle class for url_alter_test.
- *
- * Used to register an event subscriber that resolves a path alias to a system
- * path based on an arbitrary set of rules.
- *
- * @see \Drupal\url_alter_test\PathSubscriber
+ * Registers Url Alter Test module services.
  */

I don't think we should remove this additional documentation (especially the first example) and intentionally revert to "pattern documentation" on these classes. Classes can and probably should be encouraged to be more verbose. This issue was established to correct an inaccurate statement, not to force all Bundle classes to have identical documentation.

quicksketch’s picture

Status: Needs review » Needs work

This patch also removes core/modules/edit/tests/modules/lib/Drupal/edit_test/Plugin/edit/processed_text_editor/TestProcessedEditor.php entirely. Is that intentional?

Wim Leers’s picture

#10: That's probably a side effect from the patch at #1874936: Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets). The tolls of being a core contributor :)

effulgentsia’s picture

I don't understand what's useful about the 2 docs referenced in #9:

This is where Drupal core registers all of its services to the Dependency Injection Container. provides no new information relative to Registers core Drupal services.

Modules wishing to register services to the container should extend Symfony's Bundle class directly, not this class.
D'uh. If I'm gonna write a new Bundle class for my module, I'm gonna copy one from one of core's modules. Why would I even consider extending the class that registers core's services?

- * Used to register an event subscriber that resolves a path alias to a system
- * path based on an arbitrary set of rules.
- *
- * @see \Drupal\url_alter_test\PathSubscriber

What's special about this bundle class or this subscriber that makes it more worthy of this documentation than every other bundle class and service? If we think they all should receive comparable docs, then it makes sense to leave this one as setting a good example.

webchick’s picture

Issue tags: +Coding standards

Tagging so the right people see this.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.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.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.

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.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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.

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

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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.

quietone’s picture

Issue summary: View changes
Status: Needs work » Closed (duplicate)
Related issues: +#2571965: [meta] Fix PHP coding standards in core

It looks like these 'bundle' files were removed in #1939660: Use YAML as the primary means for service registration.

Thanks for the patch. However, we have a number of issues dealing with coding standards fixes and the community has decided that the best way to approach this is by fixing a rule at a time, rather than a file at a time. See #2571965: [meta] Fix PHP coding standards in core for the meta issue where this effort is being organized,

Closing this as a duplicate.

quietone’s picture

Category: Bug report » Task
Issue tags: +Bug Smash Initiative