Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Routing] Data Object Routes #175

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aarongerig
Copy link

@aarongerig aarongerig commented Dec 5, 2024

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets #163

This PR adds data object routes (Pimcore UrlSlugs) to the toolchain. It automatically generates alternate routes without the user's need to intervene.

@solverat I've opened the PR as draft, as I'm not quite sure if that's everything that needs to be done in order to cover the complete functionality the other route types already have. I'm glad if you could review and provide some feedback! I'll add documentation after the coding has been done... 😉

Copy link
Contributor

github-actions bot commented Dec 5, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@aarongerig
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@aarongerig
Copy link
Author

@solverat Got any feedback on this for me? :)

@solverat
Copy link
Member

solverat commented Mar 6, 2025

@aarongerig Finaly i have some time to test it!

First things first: Thanks for your efforts! 💪

@@ -25,13 +26,15 @@ abstract class BaseRouteItem
protected ParameterBag $routeParameters;
protected ParameterBag $routeAttributes;
protected ParameterBag $routeContext;
protected ?UrlSlug $urlSlug = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not add new properties to the route item which are very specific!

why not using the route context bag?

$routeItem->getRouteContextBag()->set('urlSlug', $urlSlug);

Comment on lines +134 to +142
public function getUrlSlug(): ?UrlSlug
{
return $this->urlSlug;
}

public function setUrlSlug(?UrlSlug $urlSlug): void
{
$this->urlSlug = $urlSlug;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this, use route context bag?

Comment on lines +55 to +58

public function getUrlSlug(): ?UrlSlug;

public function setUrlSlug(?UrlSlug $urlSlug): void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this, use route context bag?

? $routeItem->getRouteParameters()
: $routeItem->getRouteAttributes();
$object = $routeItem->getEntity();
$urlSlug = $routeItem->getUrlSlug();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use route context bag the fetch the slug?

if ($routeItem->getType() === RouteItemInterface::DATA_OBJECT_ROUTE) {
$alternateRouteItem->setRouteName($routeItem->getRouteName());
$alternateRouteItem->setEntity($routeItem->getEntity());
$alternateRouteItem->setUrlSlug($routeItem->getUrlSlug());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use route context bag the fetch the slug?

@@ -66,7 +73,8 @@ public function reverseTransformToArray(mixed $transformedRouteItem, array $cont
'routeName' => $transformedRouteItem->getRouteName(),
'routeParameters' => $transformedRouteItem->getRouteParameters(),
'routeAttributes' => $transformedRouteItem->getRouteAttributes(),
'context' => $transformedRouteItem->getRouteContext()
'context' => $transformedRouteItem->getRouteContext(),
'urlSlug' => $transformedRouteItem->getUrlSlug(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove it, urlSlug would be available in context?

} elseif (str_starts_with($currentRouteName, 'data_object_')) {
$routeItem = $this->routeItemFactory->create(RouteItemInterface::DATA_OBJECT_ROUTE, true);
$routeItem->setEntity($baseRequest->attributes->get('object'));
$routeItem->setUrlSlug($baseRequest->attributes->get('urlSlug'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use route context bag the set the slug?

'type' => RouteItemInterface::DATA_OBJECT_ROUTE,
]);

if ($routeItem->getEntity() === null || $routeItem->getUrlSlug() === null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use route context bag the fetch the slug?

@solverat solverat added this to the 5.2.0 milestone Mar 6, 2025
@solverat
Copy link
Member

solverat commented Mar 6, 2025

@aarongerig, todo:

  • add note to upgrade.md (New Feature for 5.2.0)
  • rebase to latest
  • add documentation (new document in section "Route and Alternate Links Generation")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants