Following of #3421458: Merge into Easybreadcrumb?, i create this issue that maintainers has more visibility.
Thanks to @spuky comment https://www.drupal.org/project/dynamic_breadcrumb/issues/3421458#comment...

Here is the MR link https://git.drupalcode.org/project/easy_breadcrumb/-/merge_requests/99

Issue fork easy_breadcrumb-3467372

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berramou created an issue. See original summary.

spuky made their first commit to this issue’s fork.

spuky opened merge request !131

spuky changed the visibility of the branch 2.x to hidden.

spuky’s picture

spuky commented
Status: Active » Needs review

@beramu I merged your feature Branch into the issue fork of this issue...

So we have the normal workflow...
Hope getting to to review it closer this week

Setting to needs review for anybody that wants to get involved.

spuky’s picture

spuky commented
Status: Needs review » Needs work

Ok gave it a Testdrive in one of my dev sites..

It did not change the output.

but replacement code was executed...

I guess you would have to operate on the $links array before the original

return $breadcrumb->setLinks($links)

since /core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php line 46 has an exception "Once breadcrumb links are set, only additional breadcrumb links can be added."

i guess it is not possible to change the breadcrumb from inside the Builder class... after calling setLinks on it...

also spotted a Typo... $entity_bundle_from_ink should be $entity_bundle_from_link i guess.

spuky’s picture

spuky commented

@bermaru Forget about:

It did not change the output.

but replacement code was executed...

I guess you would have to operate on the $links array before the original

return $breadcrumb->setLinks($links)

since /core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php line 46 has an exception "Once breadcrumb links are set, only additional breadcrumb links can be added."

i guess it is not possible to change the breadcrumb from inside the Builder class... after calling setLinks on it...

I forgot that we have a preprocess function running on the breadcrumb..

berramou’s picture

berramou commented

Hello @spuky thank you for the tests and review,
It's working in my local env, how did you test, how can i reproduce the warning ?

spuky’s picture

spuky commented

i used a ddev version of one of my customer sites and added the MR as a patch to composer json...:

"patches": {
 "drupal/easy_breadcrumb": {
                "Test MR:131": "https://git.drupalcode.org/project/easy_breadcrumb/-/merge_requests/131.diff"
            }
}

(the plain diff link next to the MR)

configured one node type in the dynamic breadcrumbs sektion. And used the site...

For the Taxonomy:
my vocabulary is website_catalog in the config the name gets stored as website-catalog so the case for the warning is Taxonomy Vocs with a space in the name (or a machine name containing an _ underscore...

berramou’s picture

berramou commented

To test dynamic breadcrumbs,
you need to create two content of the selected bundle i will give an example with article content

  1. Configure dynamic breadcrumbs for article for example article - [current-date:html_date]
  2. Create first article for example Article test1 with alias /article-1
  3. Create a second article for example Article test2 with alias /article-1/article-2
  4. Now if you visit the article test2 you will see that the breadcrumbs like this Home > article - 2024-08-15 > Article 2

This is what dynamic breadcrumbs does, it changes the generated part of the configured content in breadcrumb.

spuky’s picture

spuky commented

Added a suggestion that should fix The warnings on Tayonomy pages mentioned in #9 and https://git.drupalcode.org/project/easy_breadcrumb/-/merge_requests/131#...

and maybe fix the same errors with entity that have an underscore in the machine name... so $bundle_name is only uses for classes but $bundle_key to save the config...

berramou’s picture

berramou commented

Yes you are right we need to check if entity_types not configured no need to apply DB build.
Thank you for the fix i accept you suggested changes.

berramou’s picture

berramou commented

I just added the check to apply dynamic breadcrumbs only if there is an entity types configured.
TODO:

  1. Add tests for dynamic breadcrumbs part
  2. Add AJAX to the Entity Types configuration to display the appropriate settings.
  3. Adapt dynamic breadcrumb hook_requirements set the right version instead of 2.0.6
berramou’s picture

berramou commented

Still TODO:

  1. Add tests for dynamic breadcrumbs part
  2. Adapt dynamic breadcrumb hook_requirements set the right version instead of 2.0.6
  3. Add AJAX to the Entity Types configuration to display the appropriate settings.
spuky’s picture

spuky commented

I gave it another test drive... The Ajax makes config really better...

I have one thing that could make it a little touch better...

will comment on the MR

berramou’s picture

berramou commented

It seems good idea @spuky !

berramou’s picture

berramou commented

Hello @spuky any update about this improvement ?

achap made their first commit to this issue’s fork.

achap’s picture

achap
Location 🇦🇺
commented

Just resolved the merge conflict by merging in the latest stuff from 2.x so I could test it out as the patch wouldn't apply.

berramou’s picture

berramou commented

any update on this ?

greg boggs’s picture

greg boggs
he/him
Primary language English
Location Portland Oregon
commented

It looks like we need one more rebase to merge.

deaom made their first commit to this issue’s fork.

deaom’s picture

deaom commented
Status: Needs work » Needs review

Rebased branch, please re-test and re-check. Setting status to needs review.

deaom’s picture

deaom commented
Status: Needs review » Needs work

Changing back to needs work as tests are failing.

deaom’s picture

deaom commented
Status: Needs work » Needs review

Tests are passing, ready for review.

greg boggs’s picture

greg boggs
he/him
Primary language English
Location Portland Oregon
commented

woot woot! Thank you! I won't accept any more PRs until this one gets in.

berramou’s picture

berramou commented

Thank you @deaom 🚀

berramou’s picture

berramou commented

I will update #3421458: Merge into Easybreadcrumb? when it's merged to block installing Dynamic breadcrumb if the easy breadcrumb is installed with the version that contain the merge.
Thank you everyone for your contribution!