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
FileSize
880 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
FileSize
622 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.

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.

  • webchick committed e970ca8 on 8.3.x
    Issue #1864272 by dawehner, Crell: Fixed Don't use path.path_manager()...

  • webchick committed e970ca8 on 8.3.x
    Issue #1864272 by dawehner, Crell: Fixed Don't use path.path_manager()...

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.