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

Changing :slug page url parameter name breaks the setUrl methods. #25

Open
RomainMazB opened this issue Aug 28, 2022 · 3 comments
Open

Comments

@RomainMazB
Copy link
Contributor

RomainMazB commented Aug 28, 2022

All the four components of this plugin use model helper methods setUrl on Post and Category models which generate the blog or category url from the corresponding page passed in parameter of the method:

public function setUrl($pageName, $controller)
{
$params = [
'id' => $this->id,
'slug' => $this->slug
];
return $this->url = $controller->pageUrl($pageName, $params, false);

public function setUrl($pageName, $controller, $params = [])
{
$params = array_merge([
'id' => $this->id,
'slug' => $this->slug,
], $params);
if (empty($params['category'])) {
$params['category'] = $this->categories->count() ? $this->categories->first()->slug : null;
}
// Expose published year, month and day as URL parameters.
if ($this->published) {
$params['year'] = $this->published_at->format('Y');
$params['month'] = $this->published_at->format('m');
$params['day'] = $this->published_at->format('d');
}
return $this->url = $controller->pageUrl($pageName, $params);
}

The problem is that as you can see, the slug page's param is hard-coded in the url generation, which make this page url generation to fail:

// post.htm
title = "Blog post"
url = "/blog/:notSlug"
layout = "default"
is_hidden = 0

[blogPost]
slug = "{{ :notSlug }}"
categoryPage = 404
==
{% component 'blogPost' %}

Same is true for the page integrating the blogPosts, blogCategories and blogRssFeed components.

As of today, I don't see any easy way to fix this without adding a new property that would refer the target page url parameter name.
Something like this in Categories component:

// components/Categories.php

public function defineProperties()
{
    return [
        'slug' => [
            // ...
        ],
        'displayEmpty' => [
            // ...
        ],
        'categoryPage' => [
            'title'       => 'winter.blog::lang.settings.category_page',
            'description' => 'winter.blog::lang.settings.category_page_description',
            'type'        => 'dropdown',
            'default'     => 'blog/category',
            'group'       => 'winter.blog::lang.settings.group_links',
        ],
        'categoryPageSlugParam' => [
            'title'       => 'winter.blog::lang.settings.category_page_slug_param',
            'description' => 'winter.blog::lang.settings.category_page_slug_param_description',
            'default'     => 'slug',
            'type'        => 'string',
        ],
    ];
}

This property value could be passed to the setUrl method and generate the good url structure like this:

public function setUrl($pageName, $controller, $slugProp)
{
    $params = [
        'id'   => $this->id,
        $slugProp => $this->slug
    ];

    return $this->url = $controller->pageUrl($pageName, $params, false);
}
@RomainMazB
Copy link
Contributor Author

Ok, I've found how to solve this issue properly without any modification to the setUrl method, here is the example from blogPosts component:

/*
 * Add a "url" helper attribute for linking to each post and category
 */
// Load post and category pages
$activeTheme = Theme::getActiveTheme();
$postPage = Page::loadCached($activeTheme, $this->property('postPage'));
$categoryPage = Page::loadCached($activeTheme, $this->property('categoryPage'));

// Retrieve the post slug property name
if ($postPage && $postPage->hasComponent('blogPost')) {
    $postPageSlugParam = $postPage->getComponent('blogPost')->property('slug');
    if (preg_match('/^\{\{([^\}]+)\}\}$/', $postPageSlugParam, $postMatches)) {
        $postPageSlugParam = substr(trim($postMatches[1]), 1);
    }
}

// Retrieve the category slug property name
if ($categoryPage && $categoryPage->hasComponent('blogPosts')) {
    $categoryPageSlugParam = $categoryPage->getComponent('blogPosts')->property('slug');
    if (preg_match('/^\{\{([^\}]+)\}\}$/', $categoryPageSlugParam, $categoryMatches)) {
        $categoryPageSlugParam = substr(trim($categoryMatches[1]), 1);
    }
}

// Default to slug if not found
$postPageSlugParam ??= 'slug';
$categoryPageSlugParam ??= 'slug';

$posts->each(function($post) use ($postPageSlugParam, $categoryPageSlugParam, $categorySlug) {
    // Pass them as $params parameter of setUrl method
    $post->setUrl($this->postPage, $this->controller, ['category' => $categorySlug, $postPageSlugParam => $post->slug]);

    $post->categories->each(function($category) use ($categoryPageSlugParam) {
        $category->setUrl($this->categoryPage, $this->controller, [$categoryPageSlugParam => $category->slug]);
    });
});

Basically, I apply the same logic as the sitemap plugin support does here:

/*
* Extract the routing parameter name from the category filter
* eg: {{ :someRouteParam }}
*/
if (!preg_match('/^\{\{([^\}]+)\}\}$/', $properties['slug'], $matches)) {
return;
}
$paramName = substr(trim($matches[1]), 1);
$params = [
$paramName => $category->slug,

I did this directly at the component level to avoid to load 20 times the post and category page object.

If this looks good to @LukeTowers @bennothommo, I could reproduce these steps on all the blog and forum plugin's components.

@bennothommo
Copy link
Member

bennothommo commented Aug 29, 2022

@RomainMazB happy for you to provide a PR with that fix - just change the ??= operators to $variable = $variable ?? 'slug' if you can, so that we can still support Winter 1.1 with these plugins as that operator only works with PHP 7.4 and up.

@LukeTowers
Copy link
Member

@RomainMazB are you still interested in submitting a PR?

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

No branches or pull requests

3 participants