Store the file path instead of ID for the navigation logo

Created on 21 July 2024, 6 months ago
Updated 13 September 2024, 4 months ago

Problem/Motivation

Follow-up from Add a UI option to change or hide the logo on the Toolbar Needs work and split from #3436526-68: Adjust custom navigation logo dimensions on upload .

The navigation module allows you to upload a custom image for the logo, then stores the file ID.

This means that the change can only be made on production, and the site owner would then need to sync both database and files to a development environment to be able to export configuration.

The theme logo configuration also allows you to upload an image which will be stored as a managed file, but it stores the path to the image instead and has an option to manually specify the image path. This means that site owners would be able to configure this directly on a development site (even if it points to a managed file uploaded on production) without a full file sync installed.

It would also make it possible for people to use SVGs committed to their code base for the navigation icon.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Navigation 

Last updated about 19 hours ago

No maintainer
Created by

🇬🇧United Kingdom catch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • 🇬🇧United Kingdom catch

    Marked Allow SVG logos Closed: duplicate duplicate since this issue would enable svg logos to be used.

  • First commit to issue fork.
  • Assigned to pooja_sharma
  • Pipeline finished with Success
    5 months ago
    Total: 704s
    #265988
  • Issue was unassigned.
  • Status changed to Needs review 5 months ago
  • Implemented code to support image path instead of file id of navigation logo, also made some adjustments to the respective code files for the new changes. apart form nothing seems to be left.

    Please review , moving NR.

  • Status changed to Needs work 5 months ago
  • 🇺🇸United States smustgrave

    Left comments on MR.

  • Assigned to pooja_sharma
  • Thanks for reviewing , working on the it.

  • Pipeline finished with Canceled
    5 months ago
    Total: 466s
    #269591
  • Pipeline finished with Failed
    5 months ago
    Total: 685s
    #269603
  • Pipeline finished with Canceled
    5 months ago
    Total: 1781s
    #269617
  • Pipeline finished with Success
    5 months ago
    Total: 2407s
    #269644
  • Pipeline finished with Success
    5 months ago
    Total: 511s
    #270128
  • Pipeline finished with Success
    5 months ago
    Total: 637s
    #270189
  • Pipeline finished with Success
    5 months ago
    Total: 547s
    #270217
  • Issue was unassigned.
  • Status changed to Needs review 5 months ago
  • Addressed the feedback, left comment on MR. PLease review, moving NR.

  • Pipeline finished with Success
    5 months ago
    Total: 411s
    #270276
  • Status changed to Needs work 5 months ago
  • Pipeline finished with Canceled
    5 months ago
    Total: 151s
    #270346
  • 🇺🇸United States smustgrave

    Added additional comments

  • Pipeline finished with Failed
    5 months ago
    Total: 2402s
    #270347
  • Pipeline finished with Success
    5 months ago
    Total: 707s
    #270389
  • Status changed to Needs review 5 months ago
  • Addressed the feedback, left comment on MR. PLease review, moving NR.

  • 🇺🇸United States smustgrave

    I don't want to keep playing ping pong with this. Will leave for somewhere else but -1 from me as we are hard coding values and schema validation appears to be missing.

  • Assigned to pooja_sharma
  • Status changed to Needs work 5 months ago
  • On adding validation constraints appears random test failures , moving to NW 'll debug it.

  • Pipeline finished with Failed
    5 months ago
    Total: 4693s
    #270420
  • Pipeline finished with Success
    5 months ago
    Total: 749s
    #270462
  • Pipeline finished with Canceled
    5 months ago
    Total: 93s
    #270477
  • Pipeline finished with Canceled
    5 months ago
    Total: 188s
    #270479
  • Pipeline finished with Canceled
    5 months ago
    Total: 189s
    #270480
  • Pipeline finished with Success
    5 months ago
    Total: 498s
    #270481
  • hard coding values of logo extension now added in config & added constraints validation as well

    As new key logo_extension added, it leads to 'validation config' warning: https://git.drupalcode.org/issue/drupal-3462829/-/jobs/2613092

    But not getting similar warning on local even try with this command: drush config:inspect --filter-keys=navigation.settings --detail --list-constraints

    if 'll be great if anyone can share suggestion for it.

    -                "navigation.settings": 4,
    +                "navigation.settings": 5,
                     "node.settings": 2,
                     "node.type.*": 22,
                     "olivero.settings": 16,
    @@ -2788,10 +2788,10 @@
             },
             "perPropertyPath": {
                 "all": {
    -                "count": 17140
    +                "count": 17141
                 },
                 "validatable": {
    -                "count": 10377
    +                "count": 10379
                 },
                 "fullyValidatable": {
                     "count": -1
  • Issue was unassigned.
  • Status changed to Needs review 5 months ago
  • Tried to address the mentioned feedback, apart form it nothing appears to be left.

    MR is mergeable, there is one warning validation config, it display validation count & line no. increase which is correct, no any error message displaying, not sure on the basis of it, PLease review, moving to NR.

  • 🇮🇳India KumudB Ahmedabad

    There is an issue occurring when clicking on the entity used for the logo from the file system.

    1. Upload a custom logo:
    Navigate to Administration > Configuration > User Interface > Navigation > Settings.
    Upload and save your custom logo.

    2. Save the configuration:
    Ensure the changes are saved properly.

    3. Verify the issue:
    Go to Administration > Content > Files.
    Locate your most recently uploaded logo file.
    Click on the Usage link for this file.
    Observe the error that appears.

    The website encountered an unexpected error. Try again later.

    Drupal\Component\Plugin\Exception\PluginNotFoundException: The "logo" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 138 of core/lib/Drupal/Core/Entity/EntityTypeManager.php).

  • @kumudb, This issue not replicate due to these MR changes , I have verified & created issue here 🐛 Page break error display :The "logo" entity type does not exist Active , can track there.

  • 🇺🇸United States smustgrave

    Will leave for additional reviews

    But my thoughts
    But logo_managed variable name for a path doesn't make much sense to me
    Logo_extensions seems to have no field (is there not a way to get allowed file uploads?)
    The schema for logo_managed says it's allowed to be null but if it's a path to an image then should validate it's a valid path.

  • But logo_managed variable name for a path doesn't make much sense to me

    Agree , 'll working on the same, keep logo_path variable name.

    Logo_extensions seems to have no field (is there not a way to get allowed file uploads?)

    Generally when we upload the image on the node form , there is only visible image field, & image extension not visible to the user , it is attached with field storage.

    Here if we 'll add logo_extension field then on the same form logo extension & logo upload field both 'll be visible, this 'll be bit confusing experience for the user.

    The schema for logo_managed says it's allowed to be null but if it's a path to an image then should validate it's a valid path.

    On the basis of my understanding trying to rply correct me if I 'm missing anything, 'logo_managed' can be null because at the BE there are three opts available: default: (by default this value is selected when this selected then image path not get from the config, so image path not store as well, due to which I set null ) ,
    hide: in this opt no image 'll display & not store it's value in config, custom logo update: only in this opt image store, so on the basis above mentioned scenarios keep it null.

  • First commit to issue fork.
  • Pipeline finished with Failed
    4 months ago
    Total: 168s
    #278007
  • Status changed to Needs work 4 months ago
  • 🇪🇸Spain plopesc Valladolid

    Thank you for the work on this one!

    Once 📌 Adjust custom navigation logo dimensions on upload Fixed has been merged, the ground for this one is ready.

    Merged main into the MR branch and solved the conflicts as best as I could. Also added some new comments.

    Wonder if the aim of this one would be to have a similar UI and logic we already have for the theme logo, but that could be discussed later.

  • Pipeline finished with Failed
    4 months ago
    Total: 1038s
    #278010
  • Assigned to pooja_sharma
  • Pipeline finished with Failed
    4 months ago
    Total: 213s
    #278442
  • Pipeline finished with Success
    4 months ago
    Total: 764s
    #278444
  • Issue was unassigned.
  • Status changed to Needs review 4 months ago
  • Applied suggestions on MR & fixed test failures as well now MR is meregeable.

    PLease review, moving NR.

  • Status changed to Needs work 4 months ago
  • 🇪🇸Spain plopesc Valladolid

    Thank you for your help here @pooja_sharma.

    According the Issue Summary, the expected outcome of this issue is to have a similar UI and behavior to /admin/appearance/settings page, where the logo path can be defined manually, or uploading a custom file, whose path is stored in config.

    The difference with Logo image is that logic to adjust the file dimensions on upload should still be there.

  • 🇪🇸Spain plopesc Valladolid

    Marking as Navigation stable blocker because would solve indirectly 🐛 Page break error display :The "logo" entity type does not exist Active

  • 🇩🇪Germany marc.bau

    The difference with Logo image is that logic to adjust the file dimensions on upload should still be there.
    Please also do not forget that uploading SVG files must be forbidden where setting a path to a SVG file need to be allowed.

  • 🇨🇦Canada m4olivei Grimsby, ON

    I've closed 🐛 Page break error display :The "logo" entity type does not exist Active as a duplicate of this issue, since we'll resolve that here.

    Let's credit: pooja_sharma, kumudb, skaught, plopesc, and smustgrave for review / patches when we have a commit here please.

  • Merge request !9589Resolve #3462829 "Custom logo storage" → (Open) created by plopesc
  • Pipeline finished with Success
    4 months ago
    Total: 3754s
    #290977
  • Pipeline finished with Success
    4 months ago
    Total: 6335s
    #291035
  • 🇪🇸Spain plopesc Valladolid

    Created a new MR for this one where the approach is more similar to /admin/appearance/settings.

    Site administrator can enter a path or upload an image whose path will be automatically used.

  • 🇪🇸Spain plopesc Valladolid

    Attaching screenshot of the new Navigation settings UI.

  • Pipeline finished with Success
    4 months ago
    Total: 350s
    #302764
  • 🇪🇸Spain plopesc Valladolid

    MR refactored once 📌 Reorganize navigation settings to be more consistent RTBC has been merged.

  • Pipeline finished with Success
    4 months ago
    #302845
  • 🇨🇦Canada SKAUGHT

    skaught changed the visibility of the branch 3462829-store-the-file to hidden.

  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • Pipeline finished with Failed
    3 months ago
    Total: 9624s
    #306995
  • Pipeline finished with Success
    3 months ago
    Total: 551s
    #307135
  • 🇪🇸Spain plopesc Valladolid

    MR updated to solve the PHPStan and PHPCS errors after Enforce return types in all new methods and functions RTBC

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Added some questions to the MR, but don't think it should block this anyway.

  • Pipeline finished with Success
    3 months ago
    Total: 591s
    #312228
  • 🇪🇸Spain plopesc Valladolid

    Thank you for your review @penyaskito.

    Made some adjustments in the MR according to your comments.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Makes sense then!

    RTBCing.

  • 🇬🇧United Kingdom catch

    One open question on the MR, and two others I answered myself as I was asking them, otherwise looks good.

    Updating the issue summary to embed the screenshot.

  • 🇪🇸Spain plopesc Valladolid

    Thank you for the review and summary update.

    Answered the outstanding question.

  • Pipeline finished with Success
    3 months ago
    Total: 1225s
    #313276
  • Pipeline finished with Failed
    3 months ago
    Total: 154s
    #313515
  • 🇬🇧United Kingdom catch

    OK now I understand why it's there I think I've got an alternative suggestion which looks like it would be easier?

  • Pipeline finished with Success
    3 months ago
    Total: 897s
    #313551
  • 🇪🇸Spain plopesc Valladolid

    Thank you for your review @catch.

    Made a refactor of the validation and error handling logic. Hope it is good to go now.

    • catch committed 69af4bff on 11.x
      Issue #3462829 by pooja_sharma, plopesc, catch, smustgrave, penyaskito,...
  • 🇬🇧United Kingdom catch

    That looks better thank you!

    Committed/pushed to 11.x, thanks!

    I was going to cherry-pick this to 10.4.x but it doesn't apply, which suggests another change has gone into navigation 11.x which didn't get backported. Rather than rebasing this, I think we should instead try to backport that issue instead so this cherry-picks cleanly (or decide we only commit things to 11.x, but one or the other).

  • 🇬🇧United Kingdom catch
    • catch committed a4835bc8 on 10.4.x
      Issue #3462829 by pooja_sharma, plopesc, catch, smustgrave, penyaskito,...
  • 🇬🇧United Kingdom catch

    Did the cherry-pick to 10.4.x. The php baseline change didn't apply, after regenerating the baseline I discovered there were no baseline changes on 10.4.x at all, so just committed without it.

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024