Updated: Comment #0

Problem/Motivation

Drupal 7 had the useful function drupal_parse_dependency to parse dependencies declared in the "example (>7.x-2.3)" format.

This was useful for contrib and was used in Libraries API as libraries use the same mechanism to declare dependencies.

In Drupal 8 the parsing is now done in the protected function \Drupal\Core\Extension\ModuleHandler::parseDependency(). The function is therefore inaccessible to contrib and would have to be copied. This is a regression in DX.

Proposed resolution

Make \Drupal\Core\Extension\ModuleHandler::parseDependency() public.

Since it is a utility function and does not access object state at all, tstoeckler proposes that it be made static as well. This is what we do with other utility methods (see \Drupal\(Core/Component)\Utility) and so should not offend OO purists any more than existing code. I am also open to moving this method to a separate utility class in case people find that this would be "tainting" ModuleHandler.

API changes

Before:

<?php
drupal_parse_dependency
($dependency)
?>

After:

<?php
use Drupal\Core\Extension\ModuleHandler;

ModuleHandler::parseDependency($dependency)
?>

Note that there is currently no change notice for the removal of drupal_parse_dependency() :-/

Files: 
CommentFileSizeAuthor
#1 drupal8.parse-dependency-public.2068797-1.patch1.18 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,185 pass(es).
[ View ]

Comments

tstoeckler’s picture

Status:Active» Needs review
StatusFileSize
new1.18 KB
PASSED: [[SimpleTest]]: [MySQL] 58,185 pass(es).
[ View ]

Here we go.

Status:Needs review» Needs work
Issue tags:-DX (Developer Experience), -regression, -Needs change record

The last submitted patch, drupal8.parse-dependency-public.2068797-1.patch, failed testing.

tstoeckler’s picture

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience), +regression, +Needs change record

#1: drupal8.parse-dependency-public.2068797-1.patch queued for re-testing.

I think this is a random error: SearchMatchTest.php, l. 30

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

This would also make it easier to unit test it later!

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

tstoeckler’s picture

Added a change notice.

https://drupal.org/node/2090931

tstoeckler’s picture

fubhy’s picture

Sorry, but why did we make this public instead of moving it to 'Components\Utility". It has basically 0 to do with ModuleHandler as a service except that it uses it.

fubhy’s picture

Just read this:

I am also open to moving this method to a separate utility class in case people find that this would be "tainting" ModuleHandler.

So, yes, it's "tainting" ModuleHandler. I will just post a quick follow-up patch right here to move it to a utility component. Any suggestions for good name for that class?

fubhy’s picture

xjm’s picture

Issue tags:-Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

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