? Pending

User tests: Successful: Unsuccessful:

avatar cyrezdev
cyrezdev
4 Dec 2022

Pull Request for Issue #38846.

Summary of Changes

Check if taxonomy is "Type" and translate its data.

Testing Instructions

  • Needs a multi-language site.
  • Search in frontend.
  • For example with en-GB, fr-FR, de-DE, with a search for Type: Categories.

Actual result BEFORE applying this Pull Request

Before PR, Type: Category for all languages (Category not translated):

en-GB
Capture d’écran 2022-12-04 à 02 19 55

fr-FR
Capture d’écran 2022-12-04 à 02 20 54

de-DE
Capture d’écran 2022-12-04 à 02 19 43

Expected result AFTER applying this Pull Request

After PR, Category is translated:

en-GB
Capture d’écran 2022-12-04 à 02 16 16

fr-FR
Capture d’écran 2022-12-04 à 02 15 47

de-DE
Capture d’écran 2022-12-04 à 02 16 37

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar cyrezdev cyrezdev - open - 4 Dec 2022
avatar cyrezdev cyrezdev - change - 4 Dec 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Dec 2022
Category Front End com_finder
avatar cyrezdev cyrezdev - change - 4 Dec 2022
Labels Added: ?
avatar Hackwar
Hackwar - comment - 4 Dec 2022

I don't really want to have a special case just for the type, but translate everything.

avatar cyrezdev
cyrezdev - comment - 4 Dec 2022

I don't really want to have a special case just for the type, but translate everything.

What do you mean by translating everything?

other values are from item data linked to their associated language.

avatar Kostelano
Kostelano - comment - 4 Dec 2022

I confirm that it works and fixes the error.

@cyrezdev Before I submit a successful test, perhaps you can quickly fix an identical problem in the admin panel on the Smart Search: Index page. If it can't enter this PR - let me know and I'll post a successful test.

Screenshot_1

avatar cyrezdev
cyrezdev - comment - 4 Dec 2022

I confirm that it works and fixes the error.

@cyrezdev Before I submit a successful test, perhaps you can quickly fix an identical problem in the admin panel on the Smart Search: Index page. If it can't enter this PR - let me know and I'll post a successful test.

I will see to fix it in another PR (and others things i've seen to be improved or fixed in com_finder...).

So better to test this PR alone ;-)

avatar brianteeman
brianteeman - comment - 4 Dec 2022

Please keep pull requests to single tasks.

avatar Kostelano Kostelano - test_item - 4 Dec 2022 - Tested successfully
avatar Kostelano
Kostelano - comment - 4 Dec 2022

I have tested this item successfully on f2c1581


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.

avatar viocassel viocassel - test_item - 5 Dec 2022 - Tested successfully
avatar viocassel
viocassel - comment - 5 Dec 2022

I have tested this item successfully on f2c1581


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.

avatar richard67
richard67 - comment - 5 Dec 2022

I don't really want to have a special case just for the type, but translate everything.

What do you mean by translating everything?

other values are from item data linked to their associated language.

@Hackwar Could you check and report back more detailed if something is wrong or missing? Thanks in advance.

avatar cyrezdev cyrezdev - change - 5 Dec 2022
Labels Added: ?
avatar cyrezdev cyrezdev - change - 5 Dec 2022
Labels Removed: ?
avatar richard67
richard67 - comment - 5 Dec 2022

@Kostelano @viocassel Could you test again with the latest changes? Thanks in advance.

avatar cyrezdev
cyrezdev - comment - 5 Dec 2022

Thanks @richard67 for review! ?

@Kostelano @viocassel Sorry, it needs test again, as i've changed the code with error fix, and removal of language hasKey control (as already processed in branchSingular).

Could you test new version?

Thank you!

avatar viocassel viocassel - test_item - 5 Dec 2022 - Tested successfully
avatar viocassel
viocassel - comment - 5 Dec 2022

I have tested this item successfully on f98078b

?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.

avatar Kostelano Kostelano - test_item - 5 Dec 2022 - Tested successfully
avatar Kostelano
Kostelano - comment - 5 Dec 2022

I have tested this item successfully on f98078b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.

avatar richard67 richard67 - change - 5 Dec 2022
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 5 Dec 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.

avatar richard67 richard67 - test_item - 6 Dec 2022 - Tested unsuccessfully
avatar richard67
richard67 - comment - 6 Dec 2022

I have tested this item ? unsuccessfully on f98078b

Hmm, I just tested and it doesn't work for me.

@Kostelano @viocassel Are you sure you have tested according to the description in issue #38846 with the latest changes in this PR?

@cyrezdev I think you need to do it in the way like you had it at the beginning, i.e. replace the

<?php echo Text::_($text); ?>

by

<?php $key = LanguageHelper::branchSingular($text); ?>
<?php echo $lang->hasKey($key) ? Text::_($key) : $text; ?>

in the <?php if ($type === 'Type') : ?>.

I will suggest a code change on GitHub.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.
avatar richard67 richard67 - change - 6 Dec 2022
Status Ready to Commit Pending
avatar richard67
richard67 - comment - 6 Dec 2022

Back to pending due to unsuccessful test.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.

avatar Kostelano
Kostelano - comment - 6 Dec 2022

@richard67 Yes, you're right, it's broken now. I tested late at night and somehow the latest changes to the assembly did not transfer. Sorry.

I can also confirm that your suggested change works.

avatar cyrezdev
cyrezdev - comment - 6 Dec 2022

Oh i'm sorry... Maybe too much tired due to covid... :-D

So, @richard67 not needed to control hasKey as already processed in LanguageHelper::branchSingular
But, this is what i missed in my last edit... ?

I've updated this PR, accordingly to my local code...

avatar richard67
richard67 - comment - 6 Dec 2022

So, @richard67 not needed to control hasKey as already processed in LanguageHelper::branchSingular

Correct, I just saw that.

avatar cyrezdev
cyrezdev - comment - 6 Dec 2022

@Kostelano @viocassel the change was maybe not applied on your last test, so could you test again, and control code change is the same as the PR ?

Sorry for inconvenience, and thanks for your time testing! ;-)

avatar Kostelano Kostelano - test_item - 6 Dec 2022 - Tested successfully
avatar Kostelano
Kostelano - comment - 6 Dec 2022

I have tested this item successfully on 24b930a

Now it definitely works!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.

avatar richard67 richard67 - test_item - 6 Dec 2022 - Tested successfully
avatar richard67
richard67 - comment - 6 Dec 2022

I have tested this item successfully on 24b930a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39353.

avatar richard67
richard67 - comment - 6 Dec 2022

I don't really want to have a special case just for the type, but translate everything.

What do you mean by translating everything?

other values are from item data linked to their associated language.

@Hackwar Is your comment clarified with that response? If not, could you make any suggestions on what to change?

avatar richard67
richard67 - comment - 6 Dec 2022

@SharkyKZ Still not ok with this PR after it has received some changes?

avatar SharkyKZ
SharkyKZ - comment - 6 Dec 2022

Did you not see #39353 (comment)? This fixes the issue for a specific hardcoded taxonomy but does not work for other taxonomies that may be translatable in the same manner.

avatar cyrezdev
cyrezdev - comment - 6 Dec 2022

Did you not see #39353 (comment)? This fixes the issue for a specific hardcoded taxonomy but does not work for other taxonomies that may be translatable in the same manner.

What you linked to is in another PR (for admin side): #39359

Not related to this PR (frontend search results)

avatar Kostelano
Kostelano - comment - 6 Dec 2022

What you linked to is in another PR (for admin side): #39359

It's probably a third party content type. For example, in this case, with such PR logic, smart search will work for the Contacts type (it really works - I checked it), but it may NOT work with a third-party component. The type may not be translated. Not sure.

avatar cyrezdev
cyrezdev - comment - 6 Dec 2022

What you linked to is in another PR (for admin side): #39359

It's probably a third party content type. For example, in this case, with such PR logic, smart search will work for the Contacts type (it really works - I checked it), but it may NOT work with a third-party component. The type may not be translated. Not sure.

I did this PR, as i'm working on a smart search plugin for one of my extension ;-)
So, works the same (core and third-party)

avatar richard67
richard67 - comment - 6 Dec 2022

@cyrezdev What Sharky and Hackwar mean is that they don't like this $type === 'Type' special treatment. Maybe a solution could be to just leave that if away and do the echo Text::_(LanguageHelper::branchSingular($text)); always. Not sure thought if that would be right. @Hackwar Was it that what you had in mind?

avatar cyrezdev
cyrezdev - comment - 6 Dec 2022

@cyrezdev What Sharky and Hackwar mean is that they don't like this $type === 'Type' special treatment. Maybe a solution could be to just leave that if away and do the echo Text::_(LanguageHelper::branchSingular($text)); always. Not sure thought if that would be right. @Hackwar Was it that what you had in mind?

Something like this:

                            <?php $text = LanguageHelper::branchSingular(implode(',', $taxonomy_text)); ?>
                            <?php echo Text::_($text); ?>

To explain why i did apply it only on Type:

  • Type is more or less a system value (Article, Contact, Category). Not a data from $item object
  • Others are data from $item object.

In don't have problems to apply it for all, but should we allow data translation ?

For example, if you have a category with title stored in database "Contact", this title will be translated.
Because of Language key "PLG_FINDER_QUERY_FILTER_BRANCH_S_CONTACT" in plg_finder_contacts.ini

Another example, you have an article with title "March".
If translation apply to all, you will have article title translated even if you don't want it (eg. in French, it will be translated in "Mars".
Because of Language key "MARCH" in joomla.ini

So, should we translate $item data?

avatar cyrezdev
cyrezdev - comment - 6 Dec 2022

Maybe another way without checking that branch name is "Type" is to check for branch parent_id.

What needs to be translated is all branches with parent_id = 1 (with ROOT as parent).

This could work:

                            <?php $text = implode(',', $taxonomy_text); ?>
                            <?php if ($branch->parent_id == 1) : ?>
                                <?php echo Text::_(LanguageHelper::branchSingular($text)); ?>
                            <?php else : ?>
                                <?php echo $text; ?>
                            <?php endif; ?>

What's your advice? @richard67 @Hackwar @SharkyKZ

avatar richard67
richard67 - comment - 6 Dec 2022

Maybe another way without checking that branch name is "Type" is to check for branch parent_id.

What needs to be translated is all branches with parent_id = 1 (with ROOT as parent).

This could work:

                            <?php $text = implode(',', $taxonomy_text); ?>
                            <?php if ($branch->parent_id == 1) : ?>
                                <?php echo Text::_(LanguageHelper::branchSingular($text)); ?>
                            <?php else : ?>
                                <?php echo $text; ?>
                            <?php endif; ?>

What's your advice? @richard67 @Hackwar @SharkyKZ

That could work. 1 is the root item in the taxonomy table, so that should not change.

Or you could check the „level“ column, that should be 2 when parent_id is 1 because that’s the first child level of the root item, and there should not be any other items with level 1, so all level 2 are children of the root item. Maybe that’s better, I don’t really know how safely we can rely on the root item always having id = 1. if someone deletes that with SQL and rebuilds the table, it might have another id than 1.

avatar cyrezdev
cyrezdev - comment - 6 Dec 2022

Or you could check the „level“ column, that should be 2 when parent_id is 1 because that’s the first child level of the root item, and there should not be any other items with level 1, so all level 2 are children of the root item. Maybe that’s better, I don’t really know how safely we can rely on the root item always having id = 1. if someone deletes that with SQL and rebuilds the table, it might have another id than 1.

You mean level 1 ?

For me, ROOT has level 0 (zero).

avatar richard67
richard67 - comment - 6 Dec 2022

Or you could check the „level“ column, that should be 2 when parent_id is 1 because that’s the first child level of the root item, and there should not be any other items with level 1, so all level 2 are children of the root item. Maybe that’s better, I don’t really know how safely we can rely on the root item always having id = 1. if someone deletes that with SQL and rebuilds the table, it might have another id than 1.

You mean level 1 ?

For me, ROOT has level 0 (zero).

Sorry, you are right. Root item has level 0, so its direkt children have level 1.

avatar cyrezdev
cyrezdev - comment - 7 Dec 2022

Sorry, you are right. Root item has level 0, so its direkt children have level 1.

In fact, it won't work... I've tested. All taxonomy types have level 1. (Article, Category, Contact...)

I have 2 proposals:


1. Using a different way to add taxonomy when we expect its value to be translated.

For example, instead of this currently in plg_finder_content:
$item->addTaxonomy('Type', 'Article');

We can set it like this:
$item->addTaxonomy('Type', '**Article**');

And then, in search/default_results.php, code like this:

                            <?php
                            $text       = implode(',', $taxonomy_text);
                            $branchName = trim($text, '**');
                            $key        = LanguageHelper::branchSingular($branchName);
                            ?>
                            <?php if ($text !== $branchName) : ?>
                                <?php echo Text::_($key); ?>
                            <?php else : ?>
                                <?php echo $text; ?>
                            <?php endif; ?>

It requires too an update for filter (select type) to trim **:
$key = LanguageHelper::branchPlural(trim($node->title, '**'));

And for content map filter in admin:
$key = LanguageHelper::branchSingular(trim($item->text, '**'));

This way, it is backward compatible (same behavior as before with no translation, if addTaxonomy not updated with ** to be translated).


2. No change with addTaxonomy and plugins required, but won't be 100% in control.
(example case of a category with title stored in database "Contact": #39353 (comment)).

WARNING !
A concrete exemple about potential issue of the solution 2:

  • You have an article for the TV show "Friends".
  • A 3rd party finder plugin have the constant PLG_FINDER_QUERY_FILTER_BRANCH_S_FRIENDS, the title of the article will be translated as it should not.

So code in search/default_results.php:

                            <?php
                            $text = implode(',', $taxonomy_text);
                            $key  = LanguageHelper::branchSingular($text);
                            ?>
                            <?php if ($text !== $key) : ?>
                                <?php echo Text::_($key); ?>
                            <?php else : ?>
                                <?php echo $text; ?>
                            <?php endif; ?>


SUM-UP:

  • Solution 1 requires more changes, but it's B/C and it's i think a better way to have control on what could or not be translated.
  • Solution 2 is simpler, but not reliable at 100%.
  • other solution possible, but includes a refactory of com_finder, and to extend data table to manage translatable values. Not needed with solution 1.

Of course, if @richard67 @Hackwar @SharkyKZ and others agree to solution 1, i will update this PR accordingly. ;-)

avatar cyrezdev
cyrezdev - comment - 14 Dec 2022

@richard67 No news from other, but after some reflexions, i think the PR as it is, and was successfully tested, is valid.

As checking for type === 'Type'is not an issue, because changing "Type" value would be a B/C break for third-party integration, so it will stay "Type".

So, this PR to fix this issue with translation, works.

And for else (better handling of translatable or not value), it would need a refactory of com_finder, which could be set for J5.0
(and then, a $item->addTaxonomyType('article') would be better than $item->addTaxonomy('Type', 'article') i think.)

What's your opinion?

avatar richard67
richard67 - comment - 14 Dec 2022

@cyrezdev I'm not an expert on the finder. Let's see what @Hackwar says.

avatar cyrezdev
cyrezdev - comment - 14 Dec 2022

@cyrezdev I'm not an expert on the finder. Let's see what @Hackwar says.

Ok, thank you!

But i don't think he has seen the possible issue of unwanted translation, if translating everything (not only Type value).

@Hackwar this PR fixes issue for com_finder in frontend as com_finder works currently, but it would need some refactory to extend its possibilities.

Possible issue in applying translation for all:

Capture d’écran 2022-12-14 à 19 21 44

avatar cyrezdev
cyrezdev - comment - 15 Jan 2023

@richard67 @Kostelano @viocassel

As the previous PR got no news, and some requested translation for all values (#39353 (comment)), with no type check, i've redo a PR to simplify it, as i would expect this simple issue to be fixed asap! ;-)

So there the new PR: #39636

Thank you in advance for your testing! ?

avatar cyrezdev cyrezdev - change - 15 Jan 2023
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2023-01-15 14:30:15
Closed_By cyrezdev
avatar cyrezdev cyrezdev - close - 15 Jan 2023

Add a Comment

Login with GitHub to post a comment