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

Use the page name if the page has no SEO title #646

Merged
merged 1 commit into from
May 29, 2016

Conversation

core23
Copy link
Member

@core23 core23 commented May 28, 2016

Rebase of #420

Changelog

### Fixed
- The page name is now correctly used as a page title fallback

Subject

The page name fallback could never be reached because of the surrounding if-statement.

To do

  • Update the tests

@OskarStark
Copy link
Member

@greg0ire patch is correct?

@core23
Copy link
Member Author

core23 commented May 29, 2016

IMO yes, because there is a logic that will use the page name, but a wrong if-statement makes it unreachable.

@greg0ire
Copy link
Contributor

Kein deutsch bitte! "titel" => "title"

@greg0ire
Copy link
Contributor

Well I'll fix that with squash merge

@greg0ire greg0ire merged commit 10242b4 into sonata-project:3.x May 29, 2016
@greg0ire greg0ire changed the title Use the page name if the page has no SEO titel Use the page name if the page has no SEO title May 29, 2016
@greg0ire
Copy link
Contributor

Here you go! Danke schön @core23 !

@core23 core23 deleted the patch/seo-title branch May 29, 2016 13:01
@jaxel87
Copy link

jaxel87 commented Jun 30, 2016

This commit breaks seo bundle functionality. Now I can't set Seo Title values at runtime. Sonata Page Title always replace them.

$seoPage = $this->container->get('sonata.seo.page');

$seoPage->setTitle($post->getTitle())

@core23
Copy link
Member Author

core23 commented Jul 1, 2016

Can you please show a little bit more of your code?

@OskarStark
Copy link
Member

@core23 do we have a test for this?

@OskarStark OskarStark removed the RTM label Jul 1, 2016
@soullivaneuh
Copy link
Member

Maybe adding is_null($this->seoPage->getTitle()) to the condition will solve the issue? Didn't test.

@jaxel87
Copy link

jaxel87 commented Jul 1, 2016

$seoPage->setTitle($title) called 3 times. First set titile from config. Second - at runtime. Third - if page have title in admin. SeoPage Title should not be overwritte, if $page->getTitle() is null.

if ($page->getTitle()) {
    $this->seoPage->setTitle($page->getTitle());
}

if (!$this->seoPage->getTitle()) {
    $this->seoPage->setTitle($page->getName());
}

@core23
Copy link
Member Author

core23 commented Jul 1, 2016

IMHO this is another issue.

The title was always overwritten, the only thing that was changed is the fallback to the page name. Maybe you didn't have a page title, so your code just works before the patch.

core23 added a commit to core23/SonataPageBundle that referenced this pull request Jan 15, 2017
OskarStark added a commit that referenced this pull request Jan 16, 2017
Revert: Use the page name if the page has no SEO title (#646)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants