Problem/Motivation

There's a lot of classes and interfaces in the \Drupal\flag top-level namespace, making the src directory look crowded. The top level namespace should contain only top-level services, but not plugin base classes, controllers, or entity interfaces. This was done to create a parallel namespace structure with many Drupal 8 modules, but there's no current convention that says we cannot refine this further for Flag.

Proposed resolution

Perform the following moves:

  • ActionLinkTypeBase.php -> flag_root/src/ActionLink
  • ActionLinkTypePluginInterface.php -> flag_root/src/ActionLink
  • FlagAccessController.php -> flag_root/src/Access
  • FlaggingInterface.php -> flag_root/src/Entity
  • FlagInterface.php -> flag_root/src/Entity
  • FlagTypeBase.php -> flag_root/src/FlagType
  • FlagTypePluginInterface.php -> flag_root/src/FlagType

Remaining tasks

Determine if FlagLinkBuilder and FlagPermissions belong in the top level or in a subdirectory.

User interface changes

None.

API changes

Some class paths will changes.

Data model changes

None.

CommentFileSizeAuthor
#11 topNamespaceCrowded_2694755.11.patch13.57 KBsocketwench
#9 topNamespaceCrowded-2694755-9.patch87.02 KBsocketwench
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

socketwench created an issue. See original summary.

socketwench’s picture

socketwench CreditAttribution: socketwench as a volunteer commented
Issue tags: +Novice
joachim’s picture

joachim CreditAttribution: joachim commented

I'm not sure about putting things that aren't plugins in th namespace for the plugin, even if they are the base class or the interface. What do core modules do her?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Yeah, I wouldn't do that either. If doesn't break anything but it means that Drupal will try to parse those files and look for annotations on them.

I'd just introduce a new top-level namespace, e.g. ActionLink for those.

socketwench’s picture

socketwench CreditAttribution: socketwench as a volunteer commented

I was wondering if it would do that. Another top-level directory would be fine.

socketwench’s picture

socketwench CreditAttribution: socketwench as a volunteer commented
Issue summary: View changes
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Some more feedback:

* If you have ActionLink and FlagType as namespaces then you can also move the plugin managers there.

* FlagAccessController isn't really a controller, just has two access methods that are re-used. This would be better as a registered access check. See NodeAddAccessCheck for example and how it is registered in node.services.yml. You could register it as _flag_access, with the value flag/unflag. That's a lot easier to use in the various routes.

* I now moved the new storage classes into Entity/Storage, we could do the same with views data (Entity/ViewsData) and the list builder (Entity/ListBuilder, currently in Controller) classes.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Related issues: +#2695983: Refactor FlagAccessController.

For the second point of the feedback from #7 - I am spinning off a side issue.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Active » Needs review
FileSize
topNamespaceCrowded-2694755-9.patch87.02 KB

Some additions to FlagService and FlagServiceInterface due to those relying on package visibility for FlagInterface.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Note that core and most modules that I know still have the entity interfaces in the top level namespace.

I don't really care that much but that change does seem to make up a huge part of the patch.

Also, you should set up git to handle renames properly, as it's very hard to review this right now. See https://www.drupal.org/documentation/git/configure

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Issue tags: -Novice
FileSize
topNamespaceCrowded_2694755.11.patch13.57 KB

Note that core and most modules that I know still have the entity interfaces in the top level namespace.

Okay, let's not do that, since it requires a *lot* of tinkering to work. This patch just moves the plugins to subdirectories.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs review » Reviewed & tested by the community

The patch from #11 makes the module look cleaner so +1 from me.

I have one extra suggestion

looking at files in D8 that match "*Permission.php" - All similar files are stored at the same level so there is no precedent for this BUT

I want to make another namespace change.

\Drupal\flag\FlagPermissions

becomes

\Drupal\flag\Permissions\FlagPermission

That way we could remove another class from the top level.

I don't think a new coder looking at this would feel cheated if the permission class they were looking for were stored in a new Permission directory.

Anyway this is an idle thought... I am going to just RTBC.

  • socketwench authored fe198d3 on 8.x-4.x 
    Issue #2694755 by socketwench: Moved some classes out of the top level...
socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Reviewed & tested by the community » Fixed

I want to make another namespace change.

\Drupal\flag\FlagPermissions

becomes

\Drupal\flag\Permissions\FlagPermission

Sure, why not. And committed.

Status: Fixed » Closed (fixed)

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