🇺🇸United States @David_Rothstein

benjifisher → credited David_Rothstein → .

poker10 → credited David_Rothstein → .

quietone → credited David_Rothstein → .

greggles → credited David_Rothstein → .

quietone → credited David_Rothstein → .

Thanks for the reroll - I think the previous patch applied with the patch command when I tested it, but probably not with git apply since there was some fuzz.

This still needs work for the comments in #13.

1. +    watchdog('file', 'The specified file %file could not be copied to %destination.', array('%file' => $source, '%destination' => $destination));
   ....
   -      watchdog('file', 'The specified file %file could not be copied to %destination.', array('%file' => $source, '%destination' => $destination), WATCHDOG_ERROR);
   

   Why is WATCHDOG_ERROR being removed here?

2. +      watchdog('file', 'The specified file %file could not be moved to %destination.', array('%file' => $source, '%destination' => $destination));
   

   Similarly, this should probably be WATCHDOG_ERROR also, to match the above (and to match Drupal 8).

3.  function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXISTS_RENAME) {
   .....
   +  // Ensure compatibility with Windows.
   +  // @see drupal_unlink()
   +  if ((substr(PHP_OS, 0, 3) == 'WIN') && (!file_stream_wrapper_valid_scheme(file_uri_scheme($source)))) {
   +    chmod($source, 0600);
   

   I am unclear on why we'd want to make a file writable before moving it. (Currently this code only runs on a file which is about to be deleted, not renamed, right?) I would think we'd want to attempt a rename based on the current file permissions only?

4. - *   support streams if safe_mode or open_basedir are enabled. See
   - *   https://bugs.php.net/bug.php?id=60456
   + * - Works around a PHP bug where copy() does not properly support streams if
   + *   safe_mode or open_basedir are enabled.
   + *   @see https://bugs.php.net/bug.php:id=60456
   

   "See" is correct here, not "@see", since it's an inline code comment. (Using @see would cause api.drupal.org not to display it.) There are other places in the patch that have a similar problem, and it looks like Drupal 8 does also.

5. + * @param $destination
   + *   A URI containing the destination that $source should be moved/copied to.
   + *   The URI may be a bare filepath (without a scheme) and in that case the
   + *   default scheme (file://) will be used. If this value is omitted, Drupal's
   + *   default files scheme will be used, usually "public://".
   

   I don't think the comment about "default scheme (file://)" adds much here, and it's actually confusing when compared with "Drupal's default files scheme" in the next sentence. Also, the patch is introducing that comment about file:// for file_unmanaged_move() and file_unmanaged_prepare(), but not introducing it for file_unmanaged_copy(). I would expect the documentation to be consistent between all three functions.

6. + * Internal function that prepares the destination for a file_unmanaged_copy or
   + * file_unmanaged_move operation.
   + *
   + * - Checks if $source and $destination are valid and readable/writable.
   + * - Checks that $source is not equal to $destination; if they are an error
   + *   is reported.
   + * - If file already exists in $destination either the call will error out,
   + *   replace the file or rename the file based on the $replace parameter.
   

   Ideally, the first line of the docblock should be 80 characters or less and fit on one line ( https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... → ).

   Also, the list of bullets should probably have something on the line in front of it, for example "This function:".

7.   * Moves a file to a new location and update the file's database entry.
     *
   - * Moving a file is performed by copying the file to the new location and then
   - * deleting the original.
     * - Checks if $source and $destination are valid and readable/writable.
     * - Performs a file move if $source is not equal to $destination.
     * - If file already exists in $destination either the call will error out,
   

   Similarly, this should probably have something like "This function:" on the line in front of the bullets.

Overall, the patch looks like it's potentially a very nice improvement; could maybe use a review by some file system experts since this is somewhat of a big change? I'm also wondering if anyone has tested it in a real-world Drupal 7 environment, in particular some more unusual circumstances (moving files between different schemes, especially local vs. remote; on Windows; etc)...

The duplicate issue ( #1835668: Add description field to roles → ) pointed out that there's a contrib module which does this: https://www.drupal.org/project/role_help →

The linked issue is one way to do it (although probably not for Drupal 8).

Let's reopen this since the other issue wound up considering those changes to be out of scope.

There is some code in the other issue too though (in addition to the patch above). I assume it's pretty far out of date at this point though :)

(I moved it back to Drupal 8 since the Drupal 7 testbot run is finished - we still need to get an up-to-date Drupal 8 patch here so it can be fixed there first.)

I can't reproduce this in either Drupal 7 or 8.

If you look at:

https://api.drupal.org/api/drupal/modules!system!system.admin.inc/functi...
https://api.drupal.org/api/drupal/modules!system!system.admin.inc/functi...
(Drupal 7)

or:

https://api.drupal.org/api/drupal/core!modules!system!src!Form!SiteInfor...
https://api.drupal.org/api/drupal/core!modules!system!src!Form!SiteInfor...
(Drupal 8)

You can see that when the form is displayed to the administrator, the path alias (e.g. "home") with always be shown as the default value of the field, but when it is saved, the actual system path (e.g. "node/2") will be saved. Thus what is noted in #5 → :

That's exactly the problem. The config page.
I have 'node/2' aliased as 'home'.
In Site Config page I set the homepage as 'home' and it accepts it. As you say it should normalize it (change it to 'node/2') but it does not.
It behaves exactly reverse; When I try to set homepage as 'node/2' it changes it back to 'home'!

is expected behavior and does not indicate a bug. Since the system path is always saved, everything should work as intended.

Can anyone reproduce this without custom code, or otherwise explain what the bug is (in either Drupal 7 or 8)?

We discovered an issue with the patch: It calls watchdog_severity_levels() from a hook_watchdog() implementation, but watchdog is one of Drupal's bootstrap hooks which can get invoked very early in the page load, so in some cases that function won't be loaded yet so you can get a fatal error.

The attached patch fixes it by explicitly loading includes/common.inc. Note that this is rerolled based on #4 above, not #6, since #4 is what I was working off of. But I'm also attaching the interdiff showing the changes I made, so someone can apply that to #6 later.

Also setting this to "needs review" so that the testbot will run on it. (I did verify that there were test failures due to the earlier patches and the attached patch fixes the ones I saw, at least.)

In terms of the menu links part of this, I suggest we proceed with a logic similar to what chx porposed, but simplify the conditions so we only hide these special links if they have zero children. This should solve the proble for user 1 or any admin with full permission, though some junior admin with partial permissions may still potentially see empty categories.

I had a patch months ago (see #591682-27: Config pages are effectively hardcoded to some subcategory of 'admin/config' → ) that did this but at the time people did not consider it a complete solution.

Mine hid the links on output, compared to @chx's which does so on menu rebuild. I think @chx's approach is probably cleaner if it works correctly. But if we need to try a different approach, there's a starting point for that too :)

Subscribe.

Yeah, I don't understand how removing the meta categories below admin/config/* fixes this problem at all... The biggest part of this problem for D7 seems to have to do with the top level of the management menu (toolbar)... all of whose items are nicely displayed at the top of your screen, even if you don't have access to any of the items underneath some of them, right?

Subscribing. This seems like a really good idea.