larowlan → credited sun → .
@cmlara 🏆
quietone → credited sun → .
+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -132,6 +132,14 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
+ // Duplication is the act of creating a new entity based on the information
+ // that an existing entity contains. A user therefore needs to 'view' the
+ // existing entity in order to create a new one.
+ if ($operation == 'duplicate') {
+ return $this->access($entity, 'view', $langcode, $account) && $this->createAccess($entity->bundle(), $account);
+ }
Hm. This doesn't look correct to me. The base permission should be 'edit', not 'view'.
If you only have view access, then you do not know what values the entity actually contains. In order to see the (raw) values and duplicate them, you need 'edit' access.
Otherwise, you'd be able to duplicate an entity that contains a private value of another user. Previously you may only had 'view' access, because it wasn't your entity — just duplicate it, and the duplicate is yours, tadaa, Happy Easter Egg! :)
This is how it's supposed to work:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Htt...
The only addition/customization to that is that our components in Drupal\Core\* do not follow the concept of Bundles (only Drupal\Core itself is an equivalent), but as mentioned before, our plugin discovery actively supports to treat every core component as a separate unit, and we should continue to do that.
Plugin discovery is an important keyword: Console commands are pretty much the textbook example of plugins.
re: #7: @Mile23:
I cannot agree with the structure you're suggesting. Every component is supposed to be an independent, decoupled component. Components MAY ship with Console commands. If they do, then the code is provided, owned, and maintained by the component maintainers.
The only reason for why tests are split into an own namespace is that all test code should not be available and loadable at regular runtime.
Every component and every module can have Console commands. Just like they can provide e.g. Entity type plugins. Therefore, the proper home for Console commands is in a relative ./Console subnamespace/subdirectory of each component/module.
There is no need to re-invent this pattern; simply have a look at how Symfony bundles and many other PHP components (that are only using Symfony Console) are doing it. What you'll see is a ./Console subnamespace/subdirectory.
That structure allows for clean discovery of available Console commands. The defined autoload namespaces are driving the discovery. So when e.g. Guzzle ships with Console commands, then those will be automatically discovered and offered, too.
If the Console component doesn't ship with a namespace-based discovery already, then we already have plenty of our own. Just use 'em.
+1,000.
A key aspect here is a pluggable architecture.
First step towards that is to move e.g. the cache command into Drupal\Core\Cache\Console\ClearCache.
In other words, in abstract: <namespace>\Console\<Command>.php
From there on, things will become much more clear.
I'd prefer to move forward with this patch and proposed solution as is.
For potential usage of Assetic, we already have #352951: Make JS & CSS Preprocessing Pluggable → and some other issues (search for Assetic). A couple of limitations (or lacking features) in Assetic have been pointed out already.
Furthermore, for image styles, there's also #1561832: Replace Image toolkit API with Imagine → . Imagine not only covers image toolkits, but also has a built-in concept of generating image derivatives.
In turn, there's a lot to research and figure out in this field, before we can even start to think about harmonization. The idea of harmonizing all of this is a competing proposal, which should live and be discussed in its own issue, in order to not block and derail this issue.
Therefore, I'd really prefer to make pragmatic process here. That yields a concrete result and guaranteed improvement over D7.
Related, and will most likely land first: #865536: drupal_add_js() is missing the 'browsers' option →
It looks like there has been some major discussion since the last patch. It would be helpful to incorporate agreed-on adjustments to the plan in the issue summary, and also compile and add a list of remaining discussion items to it (if any).
Extremely annoying, but only borderline a bug.
I at least need the agreed on menu type constants from the patch in #92 to work around this very annoying UX problem in admin_menu.
Well. At minimum, 42% of all Drupal sites/users will care. And, depending on module decisions, various larger Drupal distro/SaaS-providers will likely care, too. In addition, the UX team cares, too.
A quick fix (hack) for those page contents makes little sense to me. Instead, we need to fix the cause - possibly by remixing/combining all previous solution attempts into a new one.
I'm not sure why you are saying that this only affects the admin/ and admin/config and similar pages.
We can turn the following in a test, and it will fail:
* Create a new user with permissions
- access administration pages
- access dashboard (required, since Dashboard contains a hack to replace access to admin/ [WTF])
* Log in that user, and go to admin/
* Neither "Structure" nor "Configuration" should be displayed in the Management menu block.
* Go to admin/config... none of the sub-categories should be displayed in the Management menu block.
Note that I mean the Management menu block. Seeing the empty categories in the page content is a derivative bug, not the cause.
Review of #591682-41: Config pages are effectively hardcoded to some subcategory of 'admin/config' →
I didn't think about the option to toggle visibility during menu router (link) building, that's interesting. I don't see the need for limiting this to parent/child access callback (mis)matches, but would rather consider to introduce a fragile and ugly hack that makes a parent item invisible in case no child is accessible -- only supporting non-dynamic children.
That would at least solve the concrete use-case and problem we have now.
EDIT: I mean, for non-dynamic children, invoke the access callbacks during menu router building. If there is a dynamic item below a MENU_GROUPING_ITEM, the parent cannot be hidden.
I wonder whether we don't have a very very similar logic with 'expanded' in http://api.drupal.org/api/function/menu_tree_page_data/7 already, which basically does what I outlined as last resort in #92.
The problem lives elsewhere and is even more fundamental than the $max_depth parameter. A screenie explains best:
There is absolutely no way to figure out whether this category/container link (Structure) has any children or any inaccessible children, because menu_tree_page_data() only loads the menu tree to the level of the link (Structure), but not anything below.
Hence, there is no way to determine whether this link should be displayed or not (because it doesn't contain or provide anything for the user) -- unless we would replace menu_tree_page_data() with menu_tree_all_data(), because the latter always builds the entire tree.
This is a fundamental problem in the menu tree building of menu_tree_page_data(), because currently, the "has_children" property of links is completely pointless. The property is TRUE even if a link has no children. My last patch contained a (commented out) fix for that in _menu_tree_data().
However, even when fixing that problem, then we still have absolutely no way to figure out whether a MENU_CONTAINER_ITEM link should be displayed, because the menu tree we have at hand does not contain any further children we could check.
The problem does not necessarily exist with menu_tree_all_data(), because that function generates the full menu tree, so we would have any possible children to determine whether a parent link should be displayed.
Brainstorming with smk-ka, we played with the idea of conditionally lazy-fetching any sub-links for MENU_CONTAINER_ITEMs when needed to determine access to it (i.e. grab all links using the mlid of the link as plid). However, those conditional queries would then run on all pages where a MENU_CONTAINER_ITEM link is visible in the tree (and has no children), which could be a performance penalty.
Another idea was to additionally fetch further children right within menu_tree_page_data() for all MENU_CONTAINER_ITEMs if not already contained in the tree result (so it would be cached with the tree) and pass that data on to menu_tree_data().
I forgot to mark as needs work, but then again, this patch including very simple tests will let the testbot do it for me.
And. Yeah. This patch is, again, an insane attempt to fix the nightmares of the menu system.
The patch fixes the actual issue, but it reveals a total flaw in a part of the menu system unrelated to this patch. Somone introduced a $max_depth parameter to menu tree data building functions without thinking about the consequence that with this forced tree limitation, the entire 'has_children' property is fucked up. As of now, no implementation is able to figure out whether a link really has no children or whether it just has been limited by the original caller. :(
pwolanin and me discussed the constant name, and having to decide between MENU_GROUPING_ITEM and MENU_CONTAINER_ITEM, we went for the latter.
And, yes, this patch would basically be back-portable to D6, because, although it introduces a new menu system type and constant, it doesn't really change the regular behavior of the original MENU_NORMAL_ITEM. The only modules and implementations that should be affected by this are menu rendering modules, and from all menu rendering modules I know, only Administration menu implemented a really awkward workaround that tried to fix the problem at least for the always visible top-level menu categories in D6 (i.e. content, build, settings, etc).
We are not going to fix those awkward system_admin_menu* functions in here. Those are total hacks, entirely disregarding menu tree functionality. Those can be fixed in #618530: System module's "listing" pages (and blocks) should use menu_build_tree() → , but not here.
This patch makes all functionality that properly uses menu tree data functions correctly output those grouping items and meta categories - which includes the "Management" menu block.
Hence, if you want to test this patch, then change your admin theme to Garland, so you see the Management menu block on all admin/* pages.
Alrighty. After a quick discussion with smk-ka, we found a really smart way to overcome the entire issue.
This patch makes the Management menu block work correctly. (and therefore also admin_menu)
After further discussion in IRC, it seems like I was totally mistaken about the purpose of MENU_DYNAMIC_ITEM in D5, and this patch still uses that constant name, but I'll change that in the next patch - probably to MENU_GROUPING_ITEM or similar. Ping me in IRC if you have a better suggestion.
I'll also look into removing all the crappy workarounds from Toolbar and System module now.
performance regression by causing access callbacks to be called on all of those links on every page
Frankly, I don't really understand why this happened on every page. I can understand that the rendering function menu_tree_output() is invoked on every page, but I don't really grok why we need to rebuild the menu tree and therefore invoke menu_tree_check_access() on every page?
ok. Recent discussion and back and forth on #591682: Config pages are effectively hardcoded to some subcategory of 'admin/config' → clarified:
- This is required for Drupal 7, as long as the meta categories below admin/config/* are not removed.
- This is required to work on a different layer: the menu system - and not only for some special-cased top-level menu items.
Regarding 1: In general though, the Toolbar will now expose a "Reports" menu item, even if the user does not have access to sub-items (or the other way around, you can't expose a report item from a contrib module without granting the permission to access all site reports), so it's not really limited to admin/config/* categories.
Regarding 2: This means that we need to re-introduce MENU_DYNAMIC_ITEM.
The menu system is totally screwed up in HEAD anyway, so having one regression more or less doesn't count.
As far as I get it, the new proposed IA for admin/config (which I do not support) would add categorization items below admin/config, which will face the very same problem: The parent item/link (category) must only be shown if the user has access to any child item. system_admin_menu_block_access() replaced the good ol' MENU_DYNAMIC_ITEM, which is what we are talking about here.
Yes. 'admin' & 'access administration pages' we need to tackle elsewhere.
FYI: Damien moved the compact mode fixes to #352734: "Compact mode" switch doesn't work for anonymous users... →
After a BIG support session with Damien in IRC, this should (hopefully) be properly. :) (Thanks again!)
This is a annoying bug - better title.
- Renamed menu item access callback to system_admin_menu_block_access(), since the additional "page" suggested that it would be used for determining access to a page, but we are just checking admin block/category access here.
- Optimized system_admin_menu_block_access() to check for the user permissions first.
- Removed all changes related to compact mode, as those are unrelated to this issue. (please create a new issue, since the changes make sense)
- Renamed test case.
Also ran tests, tested manually with a test user (and having admin menu module installed) and everything seems to working properly.
Subscribing and marking for later review. Administration menu module users are suffering from this.