8000 With the view "files" disabled, localgov_core cause an exception by marcopagliarulo · Pull Request #283 · localgovdrupal/localgov_core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

With the view "files" disabled, localgov_core cause an exception #283

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

With the view "files" disabled, localgov_core cause an exception #283

wants to merge 2 commits into from

Conversation

marcopagliarulo
Copy link
Contributor

What does this change?

Fixes #282

How to test

  1. Disable the view files
  2. Open the path admin/content/media
  3. Notice that the page is loaded properly without error and the tab files is not available
  4. Re-enable the view files
  5. Open the path admin/content/media
  6. Notice that the page is loaded properly and the tab files is now available

How can we measure success?

The media view is accessible even when the files view is not available

@ekes
Copy link
Member
ekes commented Apr 22, 2025

As this is something that can be switched on and off-again, perhaps it would be better to do it in hook_menu_local_tasks_alter()?

@marcopagliarulo
Copy link
Contributor Author

I don't think so, this solution works even when switching on and off the view. I would avoid as much as possible procedural code

@ekes
Copy link
Member
ekes commented May 1, 2025

I don't think so, this solution works even when switching on and off the view.

OK then we should probably make a test for it.

I would avoid as much as possible procedural code

https://www.drupal.org/node/3442349

@marcopagliarulo
Copy link
Contributor Author

@ekes thanks for the feedback, I'll try to reserve some time to it the coming days

ekes added a commit to localgovdrupal/localgov_guides that referenced this pull request May 6, 2025
@ekes
Follow-up issue about block caching.
28690cb 
localgovdrupal/localgov_core#283
@ekes ekes mentioned this pull request May 13, 2025
Merged
@ekes
Copy link
Member
ekes commented May 13, 2025 •
edited
Loading

I was about to make a quick test, but then see that #221 was supposed to move the files view in the menu. At least on my installs at the moment I get the menu item under content and the tab under media.

Found out this is because "This link is provided by the Admin Toolbar Extra Tools module. The title and path cannot be edited." I think we rather want to see if we can do something about that.
https://git.drupalcode.org/project/admin_toolbar/-/blob/5a0979ac832fddf3b2dc95da4044ebe4c0eecae0/admin_toolbar_tools/src/Plugin/Derivative/ExtraLinks.php#L718

So thinking aloud we want to get in later than that and change or remove that item and insert ours. I'm going to think the code on this PR could do it if we can be certain to come in later in the derivatives loop, or assuming the alter hook comes in later - that.

@ekes
Copy link
Member
ekes commented May 13, 2025

So thinking aloud we want to get in later than that and change or remove that item and insert ours. I'm going to think the code on this PR could do it if we can be certain to come in later in the derivatives loop, or assuming the alter hook comes in later - that.

OK so we can't remove the one from admin_toolbar_tools without implementing alter. So we unset that there. We can just leave the code in the MR here for adding in our own, even if it leaves menu alteration being done in two locations.

@finnlewis finnlewis requested a review from ekes May 13, 2025 11:34
ekes added a commit that referenced this pull request May 13, 2025
@ekes
Link to the files view under media. As per #221. See also #283 issues.
32c098a 
The view is supplied by localgov_media so creating the link here too.
Probably wants to be in some other LocalGov Admin configuration module
really.
@ekes ekes mentioned this pull request May 13, 2025
Merged
finnlewis pushed a commit that referenced this pull request May 27, 2025
@ekes @marcopagliarulo @stephen-cox
With the view "files" disabled, localgov_core cause an exception #283 (
0d21e01 
#297)

* With the view "files" disabled, localgov_core cause an exception

* With the view "files" disabled, localgov_core cause an exception - comment empty catch

* The intention of #221 was to move the files tab from the main content menu.

Note: This is opinionated administration configuration that probably
shouldn't be enforced on users just requiring core or media. It probably
wants to live somewhere else.

* Link to the files view under media. As per #221. See also #283 issues.

The view is supplied by localgov_media so creating the link here too.
Probably wants to be in some other LocalGov Admin configuration module
really.

* Coding standards fix

---------

Co-authored-by: Marco Pagliarulo <marcopagliarulo@gmail.com>
Co-authored-by: Stephen Cox <stephen@agile.coop>
@finnlewis
Copy link
Member

I think we can close this one, @ekes re-worked the changes on see #297

Thanks @marcopagliarulo !

@ekes
Copy link
Member
ekes commented Jun 10, 2025

Yes I think it didn't get auto closed because the commit was squashed.
But the individual commits in this branch are in the merged branch.
So thanks again @marcopagliarulo

@ekes ekes closed this Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekes ekes Awaiting requested review from ekes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

With the view "files" disabled, localgov_core cause an exception
3 participants
@marcopagliarulo @ekes @finnlewis
0