Updated: Comment #0

Problem/Motivation

We currently have ConfigMapperInterface::get(Add|Edit|Delete)Path() only to support #type operations (which uses theme_links()). Now that theme_links() supports routes, this is no longer necessary.

Proposed resolution

Remove ConfigMapperInterface::getAddPath(), ConfigMapperInterface::getEditPath(), ConfigMapperInterface::getDeletePath().

This currently does not yet remove ConfigMapperInterface::getOverviewPath().

We could also make ConfigNamesMapper::getBasePath() protected, then we would be path-free in the interface.

Remaining tasks

API changes

Remove ConfigMapperInterface::getAddPath(), ConfigMapperInterface::getEditPath(), ConfigMapperInterface::getDeletePath().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Active » Needs review
FileSize
7.89 KB

Here we go.

We still use getBasePath() for the access check, I'll look into doing that route-based now.

We still use getOverviewPath() for redirecting pack to the translation overview with ?destination=. I guess we should have route-based ?destination=, or whatever, but either way we should be able to use something like $request->requestUri() or something.

Anyway, if this is green, it should be good to go.

Gábor Hojtsy’s picture

Well, the @todo from getPathFromRoute() should also be removed then. I can do that while committing :) Awaiting green-ness.

Status: Needs review » Needs work

The last submitted patch, 1: 2136551-1.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
8.36 KB

So we need the getBasePath() for the getRequestForPath() dance. I think there is no equivalent to do that route-based because of the _system_path request attribute, which that relies on. So leaving that for now. Also not removing getOverviewPath() then for now...

Found a cooler way to do the access checking, though.

tstoeckler’s picture

Yes, #2 is correct.

Status: Needs review » Needs work

The last submitted patch, 4: 2136551-2.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
7.89 KB

There is no reason to change that access checking code for the sake of changing it :) Let's keep it as-is and minimise unrelated fail potential :)

Gábor Hojtsy’s picture

Title: Remove ConfigMapperInterface::get*Path() now that theme_links() supports route names. » Update for theme_links() supporting route names and entity link annotations being routes
FileSize
9.46 KB
1.57 KB

Also this will never pass without an entity annotations route fix as well.

Status: Needs review » Needs work

The last submitted patch, 7: 2136551-3.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Fixed
FileSize
628 bytes

Committed that with this little additional interdiff as discussed in #2.

Status: Fixed » Closed (fixed)

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