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

ACF post object titles not translated #678

Merged
merged 2 commits into from
Jul 3, 2019
Merged

Conversation

hencca
Copy link
Contributor

@hencca hencca commented Apr 29, 2019

… title

@@ -282,7 +282,7 @@ function qtranxf_before_admin_bar_render() {

function qtranxf_admin_the_title( $title ) {
//todo this filter should not be used in admin area at all?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering why this function even exists.

@@ -282,7 +282,7 @@ function qtranxf_before_admin_bar_render() {

function qtranxf_admin_the_title( $title ) {
//todo this filter should not be used in admin area at all?
if ( defined( 'DOING_AJAX' ) && DOING_AJAX )//nav-menus.php#752
if ( defined( 'DOING_AJAX' ) && DOING_AJAX && !is_admin())//nav-menus.php#752
Copy link
Collaborator

Choose a reason for hiding this comment

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

But this check is even more restrictive than the admin area, it's only for Ajax calls. Is it really what you intended?

@herrvigg
Copy link
Collaborator

Shouldn't we simply remove the whole qtranxf_admin_the_title hook?

@hencca
Copy link
Contributor Author

hencca commented Apr 30, 2019

Shouldn't we simply remove the whole qtranxf_admin_the_title hook?

That would work, but I'm new to the code base, so I don't know why it was put there.

@herrvigg
Copy link
Collaborator

herrvigg commented May 5, 2019

I won't have much time this week to look at this, but next week i'll try to.

@herrvigg
Copy link
Collaborator

@hencca sorry for such a long time but finally i tested this. I still have to better understand this old comment:

function qtranxf_admin_the_title( $title ) {
//todo this filter should not be used in admin area at all?
if ( defined( 'DOING_AJAX' ) && DOING_AJAX && !is_admin())//nav-menus.php#752
{
return $title;
}

I think the comment only applies to the AJAX case and not the whole filter function. Adding the !is_admin() test as you did is essentially like removing the whole if statement. When we enter this function we should always have is_admin() == true (but not necessarily AJAX of course). Then it continues and returns the translated stuff.

The risk is that if some async processing requires the raw data then we might do something wrong here. In some cases the raw value might be necessary, especially if it is used by a function writing data in DB! So i tend to believe we rather need to handle the AJAX request for ACF more specifically, at least in a preventive way.

Subsidiary question (but not directly related): why is there this special case for nav-menus? Not sure why this is needed. Finally, when is the_title used in the admin pages (apart from this case)?

switch ( $pagenow ) {
//case 'term.php':
//case 'edit-tags.php':
case 'nav-menus.php':
return $title;
default:
return qtranxf_useCurrentLanguageIfNotFoundUseDefaultLanguage( $title );
}

@herrvigg
Copy link
Collaborator

herrvigg commented Jul 3, 2019

@hencca all right, i finally managed to setup xdebug with Docker to better understand what's going on! This change is more complicated than expected:

  • in general we want to translate the title, the filter is used for example to display the list of posts.
  • the nav-menus want the raw values as they are handled on the client-side in Javascript (LSB).
  • the reason for the AJAX check is this fix in version 3.4.6.6 to handle the special case when a nav menu is added!

In that last case, an AJAX call is made and here we don't have the global page. So another filtering is needed, done here with DOING_AJAX and we should keep this case. But i'd say for other AJAX calls i don't think we want the raw value. If we had the autosave feature that could be a problem, but this is disabled by qTranslate...

I think i found a good solution by adding a new check on the action itself with $_REQUEST['action'] == 'add-menu-item'. I've sent a patch on your repo! This PR is updated directly so we should now be able to merge this. Note this was on the master branch (in general it's better to create separate fix branches).

@herrvigg herrvigg changed the title Added check to make sure we're not in admin area before returning raw… ACF post object titles not translated Jul 3, 2019
@herrvigg herrvigg merged commit e423f19 into qtranslate:master Jul 3, 2019
@herrvigg
Copy link
Collaborator

herrvigg commented Jul 3, 2019

@hencca i made a small mistake when committing the file, adding the execution bit (due to a mess with WSL). I managed to remove it from the main repo but not on yours. Sorry about that. Ideally you should take the new master from the reference repo. If you have problems with this the best is just to delete the file and check it out again.

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

Successfully merging this pull request may close these issues.

2 participants