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.
Comment | File | Size | Author |
---|---|---|---|
#8 | bundle_class_phpdoc.patch | 11.05 KB | effulgentsia |
#7 | bundle_class_phpdoc.patch | 12.27 KB | effulgentsia |
Comments
Comment #1
tstoecklerMy suggestion over there was simply
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
Comment #2
Wim LeersWhat about:
Pretty much every bundle's
build()
method does only call$container->register()
. That method has the following docblock:So that's where I get my proposal from.
Comment #3
quicksketch+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."
Comment #4
Wim Leers#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.Comment #5
sunJust "Registers services for the X module." or even shorter, "Registers X module services." is totally correct and fine and the way to go.
Comment #6
Wim LeersAlrighty, let's do it :)
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedComment #8
effulgentsia CreditAttribution: effulgentsia commentedOops. Rogue hunk from another issue. Cleaned up in this one.
Comment #9
quicksketchIn a few parts of effulgentsia's patch, we're deleting useful documentation:
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.
Comment #10
quicksketchThis patch also removes
core/modules/edit/tests/modules/lib/Drupal/edit_test/Plugin/edit/processed_text_editor/TestProcessedEditor.php
entirely. Is that intentional?Comment #11
Wim Leers#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 :)
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedI 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 toRegisters 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?
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.
Comment #13
webchickTagging so the right people see this.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedIt 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.
Comment #23
quietone CreditAttribution: quietone as a volunteer commented