Updated: Comment #17

Problem/Motivation

The service ID names are not greatest, but Views UI should not be referencing the path.path_manager service in its drupal_container() calls. It should be referencing path.path_manager.cached. The latter is, as you might expect, a caching wrapper around the former. Using the former directly means path lookups are not cached.

Proposed resolution

Views UI should be referencing path.path_manager.cached.

Remaining tasks

Task needs work.

User interface changes

None.

API changes

None.

Original report by crell

Files: 
CommentFileSizeAuthor
#7 drupal-1864272-7.patch622 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,618 pass(es).
[ View ]
#1 drupal-1864272-1.patch880 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,389 pass(es).
[ View ]

Comments

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new880 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,389 pass(es).
[ View ]

Here is a fast patch.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Here's a fast review.

webchick’s picture

Component:views_ui.module» documentation
Category:bug» task
Priority:Minor» Normal
Status:Reviewed & tested by the community» Active

Here's a slow commit. ;)

Committed and pushed to 8.x.

It feels like we could use a follow-up though to document this somewhere. I have no idea how people would discover it otherwise though. Hm.

jhodgdon’s picture

Hm. Do we have documentation on the drupal_container() services at all? If so, where? If not, where should we be documenting them?

dawehner’s picture

For sure, we should at least at this to the change notices, see http://drupal.org/node/1853148/revisions/view/2463318/2497390

Crell’s picture

I have updated the change notice page accordingly.

Leaving open for now, but we may want a new issue to discuss service indexing and discovery.

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new622 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,618 pass(es).
[ View ]

We could also document it on the actual manager class.

Crell’s picture

No, because there's no guarantee that class will be the class used for that service. If you run Mongo, you'll want to swap that out. That class/service separation is a major part of the point of this exercise. :-)

dawehner’s picture

Sure I totally get the point, but that's one place people will have a look at, because it's where the most used code exists,
but sure this place is not optimal.

jhodgdon’s picture

Status:Needs review» Needs work

I agree with Crell here that documenting this on the class in this way is not the right answer.

Some ideas:
- You could put documentation on the AliasManager class that says "This is the default class for the path.alias_manager service in the dependency container" or something to that effect, and the same thing for the other class. I think there's an example of this in the Transliteration classes.
- You could put @see lines linking the AliasManager class and the other one that is cached.
- You could write a topic (@defgroup) explaining the path alias classes and how to use them correctly.
- You could write or add to a documentation page in the community documentation on drupal.org.

dawehner’s picture

Could we also maybe put a service.api.php into system module which explains what service container is good for, how to use it, and explain the services provided by core?

jhodgdon’s picture

I think a @defgroup would be more appropriate than a .api.php file (that would show up as a Topic on api.drupal.org). We normally use .api.php files for hook documentation mostly.

Crell’s picture

Issue tags:-Novice

One way or another we still need to add a utility a la Symfony full stack to dump a list of services to the command line. If you're developing, you have access to that anyway. Do we need to do more than that?

Also, no longer Novice...

jhodgdon’s picture

RE #13 - that sounds like a good Drush command... ??

Crell’s picture

Given that I never succeeded in convincing someone to add the Symfony Console component to core this version, I guess we'll have to exile such useful tools to Drush for another release, yes. Sadness.

jhodgdon’s picture

PavanL’s picture

Issue summary:View changes

Added issue summary template.