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

Fix Breadcrumbs into the page #1564

Merged
merged 7 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/Admin/SharedBlockAdmin.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
use Sonata\AdminBundle\Form\FormMapper;
use Sonata\BlockBundle\Block\Service\EditableBlockService;
use Sonata\BlockBundle\Model\BlockInterface;
use Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery;
use Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQueryInterface as ORMProxyQueryInterface;
use Sonata\PageBundle\Model\PageBlockInterface;

/**
Expand All @@ -38,14 +38,20 @@ protected function generateBaseRouteName(bool $isChildAdmin = false): string
return sprintf('%s_%s', parent::generateBaseRouteName($isChildAdmin), 'shared');
}

/**
* @psalm-suppress LessSpecificReturnStatement, MoreSpecificReturnType
*
* @see https://github.com/vimeo/psalm/issues/8429
*/
protected function configureQuery(ProxyQueryInterface $query): ProxyQueryInterface
{
\assert($query instanceof ProxyQuery);
\assert($query instanceof ORMProxyQueryInterface);

// Filter on blocks without page and parents
$rootAlias = current($query->getRootAliases());
$query->andWhere($query->expr()->isNull($rootAlias.'.page'));
$query->andWhere($query->expr()->isNull($rootAlias.'.parent'));
$queryBuilder = $query->getQueryBuilder();
$rootAlias = current($queryBuilder->getRootAliases());
$queryBuilder->andWhere($queryBuilder->expr()->isNull($rootAlias.'.page'));
$queryBuilder->andWhere($queryBuilder->expr()->isNull($rootAlias.'.parent'));

return $query;
}
Expand Down
2 changes: 2 additions & 0 deletions src/Controller/PageAdminController.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ public static function getSubscribedServices(): array
}

/**
* @param ProxyQueryInterface<object> $query
Copy link
Member

Choose a reason for hiding this comment

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

But if we know this is batch for pages, shouldnt this objet be PageInterface?

cc @VincentLanglet

Copy link
Member

Choose a reason for hiding this comment

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

Indeed

*
* @throws AccessDeniedException
*/
public function batchActionSnapshot(ProxyQueryInterface $query): RedirectResponse
Expand Down
2 changes: 2 additions & 0 deletions src/Controller/SnapshotAdminController.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public static function getSubscribedServices(): array
}

/**
* @param ProxyQueryInterface<object> $query
Copy link
Member

Choose a reason for hiding this comment

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

Same but with SnapshotInterface

Copy link
Member

Choose a reason for hiding this comment

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

Indeed

*
* @throws AccessDeniedException
*/
public function batchActionToggleEnabled(ProxyQueryInterface $query): RedirectResponse
Expand Down
4 changes: 2 additions & 2 deletions src/Resources/views/layout.html.twig
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ file that was distributed with this source code.
</div>

{% block sonata_page_breadcrumb %}
<div class="row">
{{ sonata_block_render_event('breadcrumb', { 'context': 'page', 'current_uri': app.request.requestUri }) }}
<div class="row page-breadcrumb">
{{ sonata_block_render_event('breadcrumb', { 'context': 'sonata.page.block.breadcrumb', 'current_uri': app.request.requestUri }) }}
Copy link
Member

Choose a reason for hiding this comment

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

do we know why this context changed? should we revert something somewhere?

Also if this code changes is there any docs to update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check this comment please : #1564 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this bug was introduced here: https://github.com/sonata-project/SonataPageBundle/pull/1483/files#diff-f0461df8f4a53195f2b565ad37746df1e8076ff748edb18c3e2689581f2db4caR87

I guess he was just following how the stuffs were made in others parts, But in 3.x it was using page

and when added this method, it was changed!

I choose to use sonata.page.block.breadcrumb because it's more explainable then page, But yes, It's a BC!

because if someone is using sonata_block_render_event passing page, it won't work!

I could back to page, But I guess it's not the correct correct for breadcrumb, sonata.page.block.breadcrumb make more sense!

Copy link
Member

Choose a reason for hiding this comment

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

I made a mistake there, imo it should be page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well we can back this for page :),

I'll open a PR for this ;)

Copy link
Member

Choose a reason for hiding this comment

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

Yes if you can open a PR to fix the things I mentioned it would be great :) thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I opened #1570

</div>
{% endblock %}

Expand Down
57 changes: 55 additions & 2 deletions tests/Page/TemplateManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,20 @@

namespace Sonata\PageBundle\Tests\Page;

use PHPUnit\Framework\TestCase;
use Sonata\PageBundle\CmsManager\CmsSnapshotManager;
use Sonata\PageBundle\Model\PageInterface;
use Sonata\PageBundle\Model\SnapshotManagerInterface;
use Sonata\PageBundle\Model\Template;
use Sonata\PageBundle\Model\TransformerInterface;
use Sonata\PageBundle\Page\TemplateManager;
use Sonata\PageBundle\Tests\App\AppKernel;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
use Symfony\Component\DomCrawler\Crawler;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Twig\Environment;

final class TemplateManagerTest extends TestCase
final class TemplateManagerTest extends KernelTestCase
{
public function testAddSingleTemplate(): void
{
Expand Down Expand Up @@ -136,6 +144,51 @@ public function testRenderResponseWithDefaultParameters(): void
);
}

public function testTemplateShowingBreadcrumbIntoThePage(): void
{
$kernel = self::bootKernel();
// TODO: Simplify this when dropping support for Symfony 4.
// @phpstan-ignore-next-line
$container = method_exists($this, 'getContainer') ? self::getContainer() : $kernel->getContainer();

$requestStack = new RequestStack();
$requestStack->push(new Request());
$container->set('request_stack', $requestStack);

// Mocking snapshot
$pageMock = $this->createMock(PageInterface::class);
$pageMock->method('getName')->willReturn('Foo');
$pageMock->method('getParents')->willReturn([]);
$pageMock->method('getUrl')->willReturn('/');

// Mock Snapshot manager
$snapshotManagerMock = $this->createMock(SnapshotManagerInterface::class);
$transformerMock = $this->createMock(TransformerInterface::class);
$cmsSnapshotManagerMock = new CmsSnapshotManager($snapshotManagerMock, $transformerMock);
Copy link
Member

Choose a reason for hiding this comment

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

I recommend to not name variables with mock suffix , this one is a mistake for example, it is not a mock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true :/

$cmsSnapshotManagerMock->setCurrentPage($pageMock);
$container->set('sonata.page.cms.snapshot', $cmsSnapshotManagerMock);

$twig = $container->get('twig');
\assert($twig instanceof Environment);

$manager = new TemplateManager($twig, []);
$response = $manager->renderResponse('test');
/** @var string $content */
$content = $response->getContent();
$crawler = new Crawler($content);

static::assertCount(1, $crawler->filter('.page-breadcrumb'));

$breadcrumbFoo = $crawler->filter('.page-breadcrumb')->filter('a');
static::assertSame('/', $breadcrumbFoo->attr('href'));
static::assertStringContainsString('Foo', $breadcrumbFoo->text());
}

protected static function getKernelClass(): string
{
return AppKernel::class;
}

private function getTemplate(string $name, string $path = 'path/to/file'): Template
{
return new Template($name, $path);
Expand Down