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

Content sorting by date fails on multilingual site #2256

Closed
nilshoerrmann opened this issue Oct 29, 2019 · 18 comments
Closed

Content sorting by date fails on multilingual site #2256

nilshoerrmann opened this issue Oct 29, 2019 · 18 comments

Comments

@nilshoerrmann
Copy link
Contributor

@nilshoerrmann nilshoerrmann commented Oct 29, 2019

Describe the bug
We have a multilingual site (German + English) with texts that should be sorted by date. This is defined in our blueprint as follows:

title: Text
num: '{{ page.date.toDate("%Y%m%d") }}'

The date field does not allow translations and is populated in the main language only (German):

date:
  type: date
  min: 2000-01-01
  label: Datum
  translate: false
  default: today

Thus there is never a date stored inside the English content file, only inside the German one. Everything is fine as long as we edit the German version, all pages get their date prepended to their folder an sorting works as expected:

20191029_my-text

As soon as we switch to English and do any content edits, the page folder gets a zero prefix on save. My guess is that this is because Kirby tries to look up the date inside the English content file and doesn't default to the German one where the correct date can be found:

0_my-text

As soon a we edit the German version again, the folder changes back to the desired sorting prefix:

20191029_my-text

Expected behavior
Kirby should correctly look up the date in the main language file and stick to the correctly prefixed folder name.

Kirby Version
3.3.0 RC 2 – I can't remember that this happened before so I guess this is a regression between ~3.1 and now.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 29, 2019

That's strange! I'd say the Panel should always use the default language's content, no matter if translate: false is set or not. After all there can only be one sorting number for a page, so it should come from the default language to ensure consistency.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 29, 2019

I can reproduce this issue with both 3.3.0-rc.3 as well as 3.0.0, so it's not a regression. It's still a bug though. :)

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 30, 2019

I think it could be related to #2185

@nilshoerrmann

This comment has been minimized.

Copy link
Contributor Author

@nilshoerrmann nilshoerrmann commented Oct 30, 2019

Is there a quick and dirty hack we could use until a real fix has been applied?

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 30, 2019

@nilshoerrmann I have tested and i think this is not bug, just missing config, because you use strftime format %Y%m%d not date format. %Y%m%d invalid format for date() method.

return [
    'date.handler' => 'strftime',
];

You can use num: '{{ page.date.toDate("Ymd") }}' if you dont want to use strftime handler

@nilshoerrmann

This comment has been minimized.

Copy link
Contributor Author

@nilshoerrmann nilshoerrmann commented Oct 30, 2019

I've got this in my config:

return [
    'date' => [
        'handler' => 'strftime'
    ]
];

Is that an incorrect notation?
We use and need strftime on the site.

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 30, 2019

@nilshoerrmann Ok, now I understand the situation better 👍
@bastianallgeier I have tested with utf8_encode() but nothing happened so i think unrelated with #2185.

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 30, 2019

https://github.com/getkirby/kirby/blob/release/3.3.0/src/Cms/PageActions.php#L511

The $this (page) variable on this line, so the date value of the page is null on secondary language with non-translatable field.

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 30, 2019

https://github.com/getkirby/kirby/blob/release/3.3.0/src/Cms/ModelWithContent.php#L565-L566

I guess it can't get all the content (fresh data) after the reset 🤔 For example, working like that:

// reset the content object
$clone->content = $this->content();
@nilshoerrmann

This comment has been minimized.

Copy link
Contributor Author

@nilshoerrmann nilshoerrmann commented Oct 30, 2019

Looking at this code you referenced above:

case 'date':
case 'datetime':
$format = $mode === 'date' ? 'Ymd' : 'YmdHi';
$lang = $this->kirby()->defaultLanguage() ?? null;
$field = $this->content($lang)->get('date');
$date = $field->isEmpty() ? 'now' : $field;
return date($format, strtotime($date));
break;

Would num: date still work in our case?

@nilshoerrmann

This comment has been minimized.

Copy link
Contributor Author

@nilshoerrmann nilshoerrmann commented Oct 30, 2019

Okay, it seems like num: date works in our case because the used date field is actually named date.

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 30, 2019

@nilshoerrmann As you can see, using same method as $this->content() to get all content

$field  = $this->content($lang)->get('date'); 
@nilshoerrmann

This comment has been minimized.

Copy link
Contributor Author

@nilshoerrmann nilshoerrmann commented Oct 30, 2019

Using num: date is the quick fix we needed. We'll use that notation for now, which correctly defaults to the default language. 👍

So the sorting by date only breaks if the used date field is not named date and you are using a query to get it's value. Not sure if that possible with the query language but is this supposed to work?

num: '{{ page.content("de").get("date").toDate("%Y%m%d") }}'

Looks cumbersome though.

@nilshoerrmann

This comment has been minimized.

Copy link
Contributor Author

@nilshoerrmann nilshoerrmann commented Oct 30, 2019

I'm asking myself: I'm using a custom query to get my date field. Shouldn't it also be me who is taking care of the selection of the correct language because I know that field is not translatable? So maybe this is not a bug at all but a question of better documentation of how the num feature works and how to deal with it in multilingual context?

@nilshoerrmann

This comment has been minimized.

Copy link
Contributor Author

@nilshoerrmann nilshoerrmann commented Oct 30, 2019

It seems like the long query that explicitely states the default language works. So I'd say this is not a bug but a misconfiguration. The docs should be updated because there are three ways to consider when setting up date based sorting:

Sorting by a field named date

date:
  type: date
num: date

Sorting by a field of another name

datum:
  type: date
num: '{{ page.datum.toDate("Ymd") }}'

Sorting by a non-translatable field of another name in a multilingual context

default language = de

datum:
  type: date
  translate: false
num: '{{ page.content("de").get("datum").toDate("%Y%m%d") }}'
@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 30, 2019

I still think this is a bug. Kirby should parse and execute the query for the sorting number in the default language in all cases so that the user doesn't have to manually get the content of a specific language. It would then work right away without any custom modifications.

However I'm not sure how hard this would be to implement. We'd need a way to change the "current" language just for that one query execution. If that's possible – great! Otherwise it's unfortunately an issue we can't fix in the core like @nilshoerrmann explained. We should then explain this in the docs.

@bastianallgeier bastianallgeier modified the milestones: 3.3.1, 3.3.2 Nov 19, 2019
@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Nov 23, 2019

So the problem is happening here:

$template = Str::template($mode, [
    'kirby' => $this->kirby(),
    'page'  => $this,
    'site'  => $this->site(),
]);

https://github.com/getkirby/kirby/blob/master/src/Cms/PageActions.php#L509-L513

So we would need to pin the language for that one Str::template call.

@distantnative distantnative self-assigned this Nov 23, 2019
distantnative added a commit that referenced this issue Nov 23, 2019
distantnative added a commit that referenced this issue Nov 23, 2019
distantnative added a commit that referenced this issue Nov 24, 2019
@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Nov 24, 2019

@jschopplich jschopplich mentioned this issue Nov 25, 2019
0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.