Fix Breadcrumbs into the page #1564

eerison · 2022-08-18T06:17:38Z

Subject

I am targeting this branch, because it was breaking the pipeline in 4.x branch.

Changelog

### Changed
- Changed `page` context to `sonata.page.block.breadcrumb` used in `src/Resources/views/layout.html.twig` file

* Create test for templateManager * Cover breadcrumb html

into 3.x-into-4.x � Conflicts: � composer.json � tests/Page/TemplateManagerTest.php

eerison · 2022-08-18T06:30:56Z

Hey @VincentLanglet

it's the correct code for the test, But as expected it'll fail because the breadcrumbs are not returning into the page!

is it fine wait until I find out the issue, or should we skip this test and merge, and after that I open a new PR with the fix for this bug?

eerison · 2022-08-18T07:59:44Z

I guess this problem is related with: https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/Block/BreadcrumbBlockService.php

the getMenu was not used, anymore :/

VincentLanglet · 2022-08-18T08:09:38Z

Hey @VincentLanglet

it's the correct code for the test, But as expected it'll fail because the breadcrumbs are not returning into the page!

is it fine wait until I find out the issue, or should we skip this test and merge, and after that I open a new PR with the fix for this bug?

Sure, we can wait the fix

eerison · 2022-08-22T08:15:47Z

Ok as I was suspecting this bug was introduced in SEO refactoring

https://github.com/sonata-project/SonataPageBundle/pull/1483/files#diff-f0461df8f4a53195f2b565ad37746df1e8076ff748edb18c3e2689581f2db4caR87

is there any reason to use this return sonata.page.block.breadcrumb?

because it was passing page value before.

I changed the context passed by template to sonata.page.block.breadcrumb, I would say it make more sense then page

But I just would like to know if there is some thing else for this getName method with this value!

cc @Daric971

into 3.x-into-4.x

eerison · 2022-08-22T08:41:21Z

Hey @jordisala1991 could you help me here with this phpstan errors?

VincentLanglet · 2022-08-22T08:49:32Z

ProxyQuery is now generic.

ProxyQueryInterface<object>

And for

Parameter #1 $twig of class Sonata\PageBundle\Page\TemplateManager constructor expects Twig\Environment, object|null given.

You have to assert

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

src/Admin/SharedBlockAdmin.php

eerison · 2022-08-22T09:34:15Z

Ok I got those 2 errors

Error: src/Admin/SharedBlockAdmin.php:41:68: MoreSpecificReturnType: The declared return type 'Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>' for Sonata\PageBundle\Admin\SharedBlockAdmin::configureQuery is more specific than the inferred return type 'Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery&Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>' (see https://psalm.dev/070)

Error: src/Admin/SharedBlockAdmin.php:50:16: LessSpecificReturnStatement: The type 'Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery&Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>' is more general than the declared return type 'Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>' for Sonata\PageBundle\Admin\SharedBlockAdmin::configureQuery (see https://psalm.dev/12[9](https://github.com/sonata-project/SonataPageBundle/runs/7948691625?check_suite_focus=true#step:5:10))
🐑 results sent to shepherd.dev 🐑
Error: Process completed with exit code 2.

any idea how to solve @VincentLanglet

VincentLanglet · 2022-08-22T09:45:08Z

MoreSpecificReturnType: The declared return type 'Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>' for Sonata\PageBundle\Admin\SharedBlockAdmin::configureQuery is more specific than the inferred return type 'Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery&Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>

Isn't a bug ?

Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>
is considered as more specific than
Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery&Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>
but
Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery&Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>
is
Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery
AND
Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>

So to me
Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery&Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface>
is more specific than Sonata\AdminBundle\Datagrid\ProxyQueryInterface<Sonata\PageBundle\Model\PageBlockInterface> not the opposite.

Do you mind looking in the issues/creating an issue on https://github.com/vimeo/psalm/issues ?

Moreover, if it's a real issue, I think it would have been reported by phpstan.

eerison · 2022-08-22T14:24:04Z

Yeah there is a bug, But I guess it's not related with psalm

Because the code was importing a different ProxyQuery (from Datagrid), But the method returns ProxyQuery from admin

src/Admin/SharedBlockAdmin.php

VincentLanglet · 2022-08-22T16:26:53Z

Yeah there is a bug, But I guess it's not related with psalm

Because the code was importing a different ProxyQuery (from Datagrid), But the method returns ProxyQuery from admin

That's not an issue.
The ProxyQuery implements the ORMProxyQueryInterface which implements ProxyQueryInterface from AdminBundle.
So we do return a ProxyQueryInterface.

I still believe it's an issue with psalm.

src/Admin/SharedBlockAdmin.php

jordisala1991 · 2022-08-23T10:15:23Z

src/Controller/PageAdminController.php

@@ -54,6 +54,8 @@ public static function getSubscribedServices(): array
    }

    /**
+     * @param ProxyQueryInterface<object> $query


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

cc @VincentLanglet

jordisala1991 · 2022-08-23T10:16:05Z

src/Controller/SnapshotAdminController.php

@@ -35,6 +35,8 @@ public static function getSubscribedServices(): array
    }

    /**
+     * @param ProxyQueryInterface<object> $query


Same but with SnapshotInterface

jordisala1991 · 2022-08-23T10:17:13Z

src/Resources/views/layout.html.twig

-                <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 }) }}


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

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

check this comment please : #1564 (comment)

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!

I made a mistake there, imo it should be page

well we can back this for page :),

I'll open a PR for this ;)

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

I opened #1570

jordisala1991 · 2022-08-23T10:18:18Z

tests/Page/TemplateManagerTest.php

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


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

eerison added 2 commits August 17, 2022 21:52

Create test for templateManager (sonata-project#1515)

728cc5b

* Create test for templateManager * Cover breadcrumb html

Merge branch '3.x' of https://github.com/sonata-project/SonataPageBundle

af3f022

into 3.x-into-4.x � Conflicts: � composer.json � tests/Page/TemplateManagerTest.php

eerison force-pushed the 3.x-into-4.x branch 3 times, most recently from 29cb2b3 to 9f051be Compare August 18, 2022 06:28

eerison force-pushed the 3.x-into-4.x branch from 9f051be to 61f4304 Compare August 18, 2022 07:45

Fix TemplateManagerTest

9de96f2

eerison force-pushed the 3.x-into-4.x branch from 61f4304 to 9de96f2 Compare August 22, 2022 08:07

eerison added 2 commits August 22, 2022 10:16

Merge branch '4.x' of https://github.com/sonata-project/SonataPageBundle

1ecb739

into 3.x-into-4.x

Merge branch '4.x' of https://github.com/sonata-project/SonataPageBundle

0b8a3c3

into 3.x-into-4.x

eerison force-pushed the 3.x-into-4.x branch from f614f86 to 0b8a3c3 Compare August 22, 2022 08:30

Merge branch '4.x' of https://github.com/sonata-project/SonataPageBundle

695d2cb

into 3.x-into-4.x

eerison force-pushed the 3.x-into-4.x branch 2 times, most recently from 9f2fce2 to 943240e Compare August 22, 2022 09:18

VincentLanglet requested changes Aug 22, 2022

View reviewed changes

src/Admin/SharedBlockAdmin.php Outdated Show resolved Hide resolved

eerison force-pushed the 3.x-into-4.x branch from 943240e to 17e8414 Compare August 22, 2022 09:31

eerison force-pushed the 3.x-into-4.x branch 6 times, most recently from 4631a51 to ec7c892 Compare August 22, 2022 13:57

eerison force-pushed the 3.x-into-4.x branch 3 times, most recently from 5f0adf6 to 94cf542 Compare August 22, 2022 14:21

VincentLanglet requested changes Aug 22, 2022

View reviewed changes

src/Admin/SharedBlockAdmin.php Outdated Show resolved Hide resolved

src/Admin/SharedBlockAdmin.php Outdated Show resolved Hide resolved

eerison force-pushed the 3.x-into-4.x branch 3 times, most recently from 1130c52 to 903b7fb Compare August 23, 2022 06:54

sonata-project deleted a comment from eerison Aug 23, 2022

VincentLanglet requested changes Aug 23, 2022

View reviewed changes

src/Admin/SharedBlockAdmin.php Outdated Show resolved Hide resolved

Fix phpstan

205c8d5

eerison force-pushed the 3.x-into-4.x branch from 903b7fb to 205c8d5 Compare August 23, 2022 08:11

eerison marked this pull request as ready for review August 23, 2022 08:13

eerison requested a review from VincentLanglet August 23, 2022 08:13

eerison changed the title ~~3.x into 4.x~~ Fix Breadcrumbs into the page Aug 23, 2022

VincentLanglet approved these changes Aug 23, 2022

View reviewed changes

VincentLanglet merged commit b002bde into sonata-project:4.x Aug 23, 2022

eerison deleted the 3.x-into-4.x branch August 23, 2022 08:22

eerison mentioned this pull request Aug 23, 2022

Breadcrumbs are not appearing into the pages #1497

Closed

jordisala1991 reviewed Aug 23, 2022

View reviewed changes

VincentLanglet added the major label Oct 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Breadcrumbs into the page #1564

Fix Breadcrumbs into the page #1564

eerison commented Aug 18, 2022 •

edited

eerison commented Aug 18, 2022

eerison commented Aug 18, 2022

VincentLanglet commented Aug 18, 2022

eerison commented Aug 22, 2022 •

edited

eerison commented Aug 22, 2022

VincentLanglet commented Aug 22, 2022

eerison commented Aug 22, 2022

VincentLanglet commented Aug 22, 2022 •

edited

eerison commented Aug 22, 2022

VincentLanglet commented Aug 22, 2022

jordisala1991 Aug 23, 2022

VincentLanglet Aug 23, 2022

jordisala1991 Aug 23, 2022

VincentLanglet Aug 23, 2022

jordisala1991 Aug 23, 2022

eerison Aug 23, 2022

eerison Aug 23, 2022

jordisala1991 Aug 23, 2022

eerison Aug 23, 2022

jordisala1991 Aug 23, 2022

VincentLanglet Aug 23, 2022

jordisala1991 Aug 23, 2022

eerison Aug 23, 2022

Fix Breadcrumbs into the page #1564

Fix Breadcrumbs into the page #1564

Conversation

eerison commented Aug 18, 2022 • edited

Subject

Changelog

eerison commented Aug 18, 2022

eerison commented Aug 18, 2022

VincentLanglet commented Aug 18, 2022

eerison commented Aug 22, 2022 • edited

eerison commented Aug 22, 2022

VincentLanglet commented Aug 22, 2022

eerison commented Aug 22, 2022

VincentLanglet commented Aug 22, 2022 • edited

eerison commented Aug 22, 2022

VincentLanglet commented Aug 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerison commented Aug 18, 2022 •

edited

eerison commented Aug 22, 2022 •

edited

VincentLanglet commented Aug 22, 2022 •

edited