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

Make title different from meta title on CMS for SEO purpose #9475

Merged
merged 2 commits into from Sep 6, 2018

Conversation

@Mreker
Contributor

Mreker commented Aug 22, 2018

Questions Answers
Branch? develop
Description? Read ticket.
Type? bug fix
Category? CO
BC breaks? No, but a line of code in theme is needed to make this work.
Deprecations? No
Fixed ticket? http://forge.prestashop.com/browse/BOOM-5262?filter=-2
How to test? You should be able to set a meta title different from title. Try with string length higher than 255

This change is Reviewable

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 22, 2018

Contributor

@jolelievre I see you do a lot of SEO work atm. Just want to let you know of this PR, so you don't do the work as well 👍

Contributor

Mreker commented Aug 22, 2018

@jolelievre I see you do a lot of SEO work atm. Just want to let you know of this PR, so you don't do the work as well 👍

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Aug 22, 2018

Member

Note this PR is incomplete. If the structure of the database is altered, you must provide an equivalent in install-dev/upgrade/sql/1.7.5.0.sql for people upgrading their shops. :)

Member

Quetzacoalt91 commented Aug 22, 2018

Note this PR is incomplete. If the structure of the database is altered, you must provide an equivalent in install-dev/upgrade/sql/1.7.5.0.sql for people upgrading their shops. :)

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 22, 2018

Contributor

ok thanks for the update! I just don't understand the purpose of SEO improvement in the BO, shouldn't it be ignored by SEO bots?

Contributor

jolelievre commented Aug 22, 2018

ok thanks for the update! I just don't understand the purpose of SEO improvement in the BO, shouldn't it be ignored by SEO bots?

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 22, 2018

Contributor

@jolelievre This PR make it possible to make a meta title like "Delivery - Our delivery is fast and secure...." and page title: "Delivery"

Without this PR title = meta title

Contributor

Mreker commented Aug 22, 2018

@jolelievre This PR make it possible to make a meta title like "Delivery - Our delivery is fast and secure...." and page title: "Delivery"

Without this PR title = meta title

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 22, 2018

Contributor

Ok I get it
But you chose to use the new cms_title as the required/fallback field, wouldn't it be more logical to keep the existing field meta_title as the default one And cms_title is only used to override it in the page title?

Contributor

jolelievre commented Aug 22, 2018

Ok I get it
But you chose to use the new cms_title as the required/fallback field, wouldn't it be more logical to keep the existing field meta_title as the default one And cms_title is only used to override it in the page title?

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 22, 2018

Contributor

I use same behavior as products, category etc. I think this is the right behavior. Did you test it? :-)

add database field:

SET SESSION sql_mode = '';
SET NAMES 'utf8';

ALTER TABLE `PREFIX_cms_category_lang` ADD `cms_title` VARCHAR(128) NULL DEFAULT NULL; 
Contributor

Mreker commented Aug 22, 2018

I use same behavior as products, category etc. I think this is the right behavior. Did you test it? :-)

add database field:

SET SESSION sql_mode = '';
SET NAMES 'utf8';

ALTER TABLE `PREFIX_cms_category_lang` ADD `cms_title` VARCHAR(128) NULL DEFAULT NULL; 
@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 22, 2018

Contributor

I made a new PR because of some github problems: #9476

How do I close this?

Contributor

Mreker commented Aug 22, 2018

I made a new PR because of some github problems: #9476

How do I close this?

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 22, 2018

Contributor

@Quetzacoalt91 fixed missig sql part #9476. Can you close this PR?

Contributor

Mreker commented Aug 22, 2018

@Quetzacoalt91 fixed missig sql part #9476. Can you close this PR?

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 22, 2018

Contributor

@Mreker it is not exactly the same behaviour for products or categories because the title are based on the name and attributes
Morover cms_title is the common field between all ObjectModels that's why it should be used as the default container, if not users with former CMS pages will get an error when they want to update a page if they don't fill the new cms_title field, this could be confusing for them

Contributor

jolelievre commented Aug 22, 2018

@Mreker it is not exactly the same behaviour for products or categories because the title are based on the name and attributes
Morover cms_title is the common field between all ObjectModels that's why it should be used as the default container, if not users with former CMS pages will get an error when they want to update a page if they don't fill the new cms_title field, this could be confusing for them

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 22, 2018

Contributor

I think I will rather solve this in 1.7.5.0.sql

Contributor

Mreker commented Aug 22, 2018

I think I will rather solve this in 1.7.5.0.sql

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 22, 2018

Contributor

you mean you want to copy the content of meta_title field inside cms_title field to make sure it is not empty?

Contributor

jolelievre commented Aug 22, 2018

you mean you want to copy the content of meta_title field inside cms_title field to make sure it is not empty?

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 22, 2018

Contributor

yes

Contributor

Mreker commented Aug 22, 2018

yes

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 22, 2018

Contributor

Then it seems to complicate your upgrade script as well
If you keep the initial field as default value you only need to alter the table with the new optional field, simpler upgrade and it avoids misunderstanding for users

Contributor

jolelievre commented Aug 22, 2018

Then it seems to complicate your upgrade script as well
If you keep the initial field as default value you only need to alter the table with the new optional field, simpler upgrade and it avoids misunderstanding for users

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 23, 2018

Contributor

I agree with you, I will make the change in order to make the change better backward compatible.

Contributor

Mreker commented Aug 23, 2018

I agree with you, I will make the change in order to make the change better backward compatible.

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 23, 2018

Contributor

great! thanks
you will probably have to rebase your branch as well, we merged a huge PR yesterday for coding styles purposes, this results in a lot of conflicts sadly

Contributor

jolelievre commented Aug 23, 2018

great! thanks
you will probably have to rebase your branch as well, we merged a huge PR yesterday for coding styles purposes, this results in a lot of conflicts sadly

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 23, 2018

Contributor

you want the new field to be seo_meta_title then? and keep meta_title for title?

Contributor

Mreker commented Aug 23, 2018

you want the new field to be seo_meta_title then? and keep meta_title for title?

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 23, 2018

Contributor

I was thinking the opposite, meta_title is used for the meta title (in the <title> tag) like in other pages
And your new field cms_title is used for the page title (<h1> tag) and is optional, if left empty it is filled with meta_title in classes/Meta.php

- $row['meta_title'] = empty($row['meta_title']) ? $row['cms_title'] : $row['meta_title'];
+ $row['cms_title'] = empty($row['cms_title']) ? $row['meta_title'] : $row['cms_title'];

I wonder if cms_title should be renamed in page_title? Maybe for non technical users it would be easier to understand, what do you think?

Contributor

jolelievre commented Aug 23, 2018

I was thinking the opposite, meta_title is used for the meta title (in the <title> tag) like in other pages
And your new field cms_title is used for the page title (<h1> tag) and is optional, if left empty it is filled with meta_title in classes/Meta.php

- $row['meta_title'] = empty($row['meta_title']) ? $row['cms_title'] : $row['meta_title'];
+ $row['cms_title'] = empty($row['cms_title']) ? $row['meta_title'] : $row['cms_title'];

I wonder if cms_title should be renamed in page_title? Maybe for non technical users it would be easier to understand, what do you think?

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 23, 2018

Contributor

I see your point in order to avoid some sql code. The issue I see the most is that everywhere title should be displayed, meta_title is used. This mean, that if we want to keep meta_title for meta title and add cms_title for title, then we need to rename the every tpl file that use meta_title.

To avoid rename every tpl files we could make an ugly solution like meta_title for title and seo_meta_title for meta title. This will avoid a BC break.

You get my point?

Contributor

Mreker commented Aug 23, 2018

I see your point in order to avoid some sql code. The issue I see the most is that everywhere title should be displayed, meta_title is used. This mean, that if we want to keep meta_title for meta title and add cms_title for title, then we need to rename the every tpl file that use meta_title.

To avoid rename every tpl files we could make an ugly solution like meta_title for title and seo_meta_title for meta title. This will avoid a BC break.

You get my point?

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 23, 2018

Contributor

I think the right solution is to rename everything and make title required and meta title not. As meta description is not required as well.

For the database part, the meta title content should be moved to title content.

Contributor

Mreker commented Aug 23, 2018

I think the right solution is to rename everything and make title required and meta title not. As meta description is not required as well.

For the database part, the meta title content should be moved to title content.

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 23, 2018

Contributor

are you sure? I think the modification you made in themes/classic/templates/cms/page.tpl works just fine

{block name='page_title'}
    {$cms.cms_title}
{/block}

page_title => h1 tag ib the cms page, right ?

so you don't need to change any other tmp file, because anyway the cms_title new field is only added for cms page, the other content use different tables and templates

Contributor

jolelievre commented Aug 23, 2018

are you sure? I think the modification you made in themes/classic/templates/cms/page.tpl works just fine

{block name='page_title'}
    {$cms.cms_title}
{/block}

page_title => h1 tag ib the cms page, right ?

so you don't need to change any other tmp file, because anyway the cms_title new field is only added for cms page, the other content use different tables and templates

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 23, 2018

Contributor

bo
fo

Contributor

Mreker commented Aug 23, 2018

bo
fo

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 23, 2018

Contributor

yes that's because the initial meta_title field is used in the breadcrumbs, as well as in the admin
I think meta_tile is maybe not the best name since it might confuse people, but it is the same for every Product/Category/CMS pages.. so we can't modify it for now as it would break too many compatibilities
I think I get everything better now, correct me if I am wrong, but your initial aim is to set the meta title (title tag) different from the page one (h1)

Then I think page title (h1) should have the same value as the one in breadcrumbs for consistency and thus must be stored in the original meta_title field, also you shouldn't change the order in the form, nor the required value and especially not the translation or it will be confusing

As for the meta title override, I think what you proposed with seo_meta_title was the right approach, you should add this field as an optional field (default value is filled from meta_title) Then the page template should be reverted to use meta_title in the page title And the one you need to override is:

{block name='head_seo_title'}{$cms.seo_meta_title}{/block}

Then base on how the blocks are named, your seo_meta_title should be named head_seo_title for consistency and understanding

I hope my idea is clear ^^ I kinda wrote it as I was thinking it What do you think?

Contributor

jolelievre commented Aug 23, 2018

yes that's because the initial meta_title field is used in the breadcrumbs, as well as in the admin
I think meta_tile is maybe not the best name since it might confuse people, but it is the same for every Product/Category/CMS pages.. so we can't modify it for now as it would break too many compatibilities
I think I get everything better now, correct me if I am wrong, but your initial aim is to set the meta title (title tag) different from the page one (h1)

Then I think page title (h1) should have the same value as the one in breadcrumbs for consistency and thus must be stored in the original meta_title field, also you shouldn't change the order in the form, nor the required value and especially not the translation or it will be confusing

As for the meta title override, I think what you proposed with seo_meta_title was the right approach, you should add this field as an optional field (default value is filled from meta_title) Then the page template should be reverted to use meta_title in the page title And the one you need to override is:

{block name='head_seo_title'}{$cms.seo_meta_title}{/block}

Then base on how the blocks are named, your seo_meta_title should be named head_seo_title for consistency and understanding

I hope my idea is clear ^^ I kinda wrote it as I was thinking it What do you think?

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 23, 2018

Contributor

I tried to solve it, but run into some strange problems.

Can you please give it a shot? :-) I think you already got a solution.

Contributor

Mreker commented Aug 23, 2018

I tried to solve it, but run into some strange problems.

Can you please give it a shot? :-) I think you already got a solution.

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 24, 2018

Contributor

sure, I'll take a look this afternoon

Contributor

jolelievre commented Aug 24, 2018

sure, I'll take a look this afternoon

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 24, 2018

Contributor

Can I close #9476? =)

Contributor

PierreRambaud commented Aug 24, 2018

Can I close #9476? =)

@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Aug 24, 2018

Contributor

@jolelievre 👍

@PierreRambaud yes please

Contributor

Mreker commented Aug 24, 2018

@jolelievre 👍

@PierreRambaud yes please

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 30, 2018

Contributor

You forget to run php-cs-fixer ;) Maybe you should come on emacs ;)

Contributor

PierreRambaud commented Aug 30, 2018

You forget to run php-cs-fixer ;) Maybe you should come on emacs ;)

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 30, 2018

Contributor

damn! and done! ^^

Contributor

jolelievre commented Aug 30, 2018

damn! and done! ^^

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Aug 31, 2018

@marionf marionf self-assigned this Aug 31, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Aug 31, 2018

@marionf marionf removed their assignment Aug 31, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 31, 2018

Contributor

@jolelievre A little rebase and merge :)

Contributor

PierreRambaud commented Aug 31, 2018

@jolelievre A little rebase and merge :)

'name' => 'meta_title',
'id' => 'name', // for copyMeta2friendlyURL compatibility
'lang' => true,
'required' => true,
'class' => 'copyMeta2friendlyURL',
'hint' => $this->trans('Invalid characters:', array(), 'Admin.Notifications.Info') . ' &lt;&gt;;=#{}',
'hint' => array(
$this->trans('Used in the h1 page tag, and as the default title tag value.', array(), 'Admin.Design.Help'),

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Aug 31, 2018

Contributor

Will this wording be a tooltip?

@LouiseBonnard

LouiseBonnard Aug 31, 2018

Contributor

Will this wording be a tooltip?

This comment has been minimized.

@Mreker

Mreker Aug 31, 2018

Contributor

Yes it is

@Mreker

Mreker Aug 31, 2018

Contributor

Yes it is

This comment has been minimized.

@Mreker
@Mreker

Mreker Sep 1, 2018

Contributor

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Sep 3, 2018

Contributor

Perfect domains then, thanks @Mreker!

@LouiseBonnard

LouiseBonnard Sep 3, 2018

Contributor

Perfect domains then, thanks @Mreker!

Show outdated Hide outdated controllers/admin/AdminCmsController.php Outdated
@Mreker

This comment has been minimized.

Show comment
Hide comment
@Mreker

Mreker Sep 1, 2018

Contributor

@jolelievre you can take credit for this PR :-) Thanks.

Contributor

Mreker commented Sep 1, 2018

@jolelievre you can take credit for this PR :-) Thanks.

Show outdated Hide outdated classes/CMS.php Outdated
@PierreRambaud

Little changes needed

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Sep 6, 2018

Contributor

@PierreRambaud sorry I made a rebase and now I can't see you requested changes anymore.. Could you check if there are still some corrections to make please? Thanks

Contributor

jolelievre commented Sep 6, 2018

@PierreRambaud sorry I made a rebase and now I can't see you requested changes anymore.. Could you check if there are still some corrections to make please? Thanks

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Sep 6, 2018

Contributor

Hum weird rebase, you include some @mickaelandrieu changes :/

Contributor

PierreRambaud commented Sep 6, 2018

Hum weird rebase, you include some @mickaelandrieu changes :/

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Sep 6, 2018

Contributor

wow you're right!! much better now!

Contributor

jolelievre commented Sep 6, 2018

wow you're right!! much better now!

@jolelievre jolelievre merged commit caf2655 into PrestaShop:develop Sep 6, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Sep 6, 2018

Contributor

all done and merged! thanks @Mreker !!

Contributor

jolelievre commented Sep 6, 2018

all done and merged! thanks @Mreker !!

@Mreker Mreker deleted the Mreker:Fix-meta-field-on-CMS branch Sep 7, 2018

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