Switch from user login block to login menu link and search block in standard profile

Looks great @bobrov1989! Thanks.

I asked myself whether it makes sense to disable the current user login block (as you did in the patch) or to remove it alltoghether. Not sure, thoughts?

One minor hick-up in the patch:

+++ b/core/profiles/standard/standard.links.menu.yml
@@ -2,3 +2,7 @@ standard.front_page:
+  menu_name: account
\ No newline at end of file

Please add a newline to the end of the file.

SQLSTATE[HY000]: General error: 13 database or disk is full

...requested a re-test.

@bobrov1989 the patch looks great now from a code review. I have yet to try it out on a fresh install, so not setting to RTBC. Thanks for the quick follow-up!

Only issue that i can see with latest patch -- login link showing when you are already logged in + text should be just " log in " rather than login.

+++ b/core/profiles/standard/standard.links.menu.yml
@@ -2,3 +2,9 @@ standard.front_page:
+  base_route: user.page

You don't need base_route in menu links, just skip that line. route_name is enough to begin with

This takes up quite a lot of screen space (especially on mobile)

This is IMO the best justification.

This sounds good to me :) Definitely an annoying thing on mobile.

The only thing that the new place is quite undiscoverable.

Addressed #31.

Forgot to upload the interdiff.

maybe there's existing CR that needs update
+1 RTBC

CR created: https://www.drupal.org/node/2554545.

Looks good

Some images. Possibly needs more styling. Why make it part of User Account Menu? Actually looks a little weird to me. Maybe part of Main menu, hopefully not visible when logged in?

Thanks for the screenshots!

It probably only looks weird because you've never seen that menu up there? That's how it's been styled since forever, including in Drupal 7. Nothing weird about it. Just different than what you've grown used to :)

But when the login form is displayed the text for the 'log in' menu is black when it should be white. That's why I changed it to needs work.

But when the login form is displayed the text for the 'log in' menu is black when it should be white. That's why I changed it to needs work.

That may not be the prettiest, but it is definitely not a bug; it's intentional that we style the active link differently. Looks like Bartik could use an override for this though. But can we please first get feedback from @webchick on whether this is an acceptable change overall? I'm sure she'll kick it back for this very reason, but before fixing that by getting Bartik people involved, it'd be useful to know whether she considers this an acceptable change or not.

From core/modules/system/css/components/menu.theme.css:

ul.menu a.is-active {
  color: #000;
}

One thing I missed and that should be fixed first: https://www.drupal.org/files/issues/loggedin.jpg shows that the "Log in" link is still present for a logged in user. That's not acceptable.

This may be working as designed. The log in link only appears on pages for users with the 'link to any page' permission. This is on for the administrative account on a default install. If you create a standard user account you won't see the login menu item when logged in. See #2463753: [regression] Do not bypass route access with 'link to any page' permissions for menu links and #2267603: "Login" link shows up for logged-in users as well for discussion.

I would like to argue to add the 'log in' link to the 'main' menu rather than the 'user account' menu. The login link looks a little lost way up their in the corner(unless you're on a mobile device).

This may be working as designed. The log in link only appears on pages for users with the 'link to any page' permission. This is on for the administrative account on a default install.

Oh! Hah!

Another issue that left sidebar is always displayed, so .layout-sidebar-first .main-content breaks layout

#50:

can we please first get feedback from @webchick on whether this is an acceptable change overall?

Also, that's a pre-existing bug: #953034: [meta] Themes improperly check renderable arrays when determining visibility.

#50 A new user has hardly any permissions, not even search, though I don't know why the 'Tools' menu doesn't show up.

#52: #50 is the anon user. The anon user doesn't get have permission to use any of the links in the 'Tools' menu. The authenticated user does get to see the 'Tools' menu.

Overall this would be a good change: less ui bloat, easier on the eyes and on page load, not only on mobile.

The login link looks to be in the right place as well. Top right is a very common place to put it. Having both 'Log in' and 'Log out' links show at the same time is weird, even if that is as designed, would be nice to have that not be the case. And a fix for Bartik to not show the active state as black would be needed as well.

#54: many thanks for your review!

Having both 'Log in' and 'Log out' links show at the same time is weird, even if that is as designed, would be nice to have that not be the case.

It only affects user 1, or any user with the permission that has permission to "link to any page".

And a fix for Bartik to not show the active state as black would be needed as well.

Agreed, but let's first hear what @webchick thinks about this overall change, minus the cosmetic problems.

So doing a quick survey of some of http://www.alexa.com/topsites in Ingocnito mode...

- https://www.google.ca / https://www.youtube.com/: "Sign in" button, upper right.

- https://www.facebook.com/: Log in / Sign up form immediately visible on home page, on right:

- https://ca.yahoo.com/: "Sign in" iconed-button, top right.

- https://www.amazon.com/ is weird, but once again top-right:

- https://en.wikipedia.org/wiki/Main_Page: create account / log in in top right:

- https://twitter.com/: Log in / Sign up form immediately visible

Based on this, the trends clearly indicate:

1. Authentication stuff is basically always on the right. Most often, the upper-right.
2. Only social networking sites which have basically _zero_ functionality for anonymous users expose a login block by default.
3. Even when they do this, it's only done on the home page, not on other pages such as "About Us."

So it's pretty clear to me that a) Drupal is breaking best practices atm (it does a login block on the left, on all pages, which none of the other "top 10" sites do), and b) it's also making assumptions about the types of sites that are being built with Drupal as being mainly social networking where authentication is the major use case (which based on the usage stats of modules such as https://www.drupal.org/project/user_relationships do not seem to bear out being the 90% (or even 10%) use case).

So I'm definitely on board with kicking the block out of standard profile and moving to a link instead.

Not very on board though with exposing both Log in and Log off, even to user 1. That's a UX regression, breaking all of these web patterns. In testing, it seems to redirect user/login to user/1, so can we just re-title the Log in link to "My account" for privileged users?

Also would be great follow-up to login link in modal (maybe contrib module)

One or both of these should address the UX regression.

Thanks webchick! Good idea to rename the 'Login' link shown to a logged-in user 1 to "My account". Needs work for that.

That's a UX regression, breaking all of these web patterns. In testing, it seems to redirect user/login to user/1, so can we just re-title the Log in link to "My account" for privileged users?

That'd mean they'd end up getting a redirect every single time they want to go to their profile. If I need to make the title change dynamically for authenticated users, then I might just as well change the route name too.

Other than that detail, this patch does exactly that.

This patch also fixes the CSS problem.

So IMO the problem with the the patch in #60 is that it's relatively confusing to have a "My account" link that turns into "Log in". Wouldn't it be better to the "Log in" link turn into "Log out" and vice versa? That seems much more logical.

"Wouldn't it be better to the "Log in" link turn into "Log out" and vice versa? That seems much more logical."

Oh, yes, that would be fantastic. :D You should only ever see one or the other.

You should only ever see one or the other.

My thoughts exactly :)

This should be green!

+++ b/core/modules/menu_ui/src/MenuForm.php
@@ -357,6 +357,9 @@ protected function buildOverviewTreeForm($tree, $delta) {
           $form[$id]['title']['#markup'] .= ' (' . $this->t('disabled') . ')';
...
+        elseif ($id === 'menu_plugin_id:user.login.logout') {
+          $form[$id]['title']['#markup'] .= ' (' . $this->t('<q>Log in</q> for anonymous users') . ')';
...
           $form[$id]['title']['#markup'] .= ' (' . $this->t('logged in users only') . ')';

Any reason for quotation tag? and other lines are small caps

Because it says Log out for the current user, since the current user is always going to be authenticated if they can view this UI. So I want it the UI to be clear, and therefore show "Log out (Log in for anonymous users)".

#2529560: Expand support for link objects landed yesterday, that caused those exceptions.

This should finally be green!

#2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) landed this morning, which caused the patch to not apply anymore.

#2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) landed, which also adds the config:system.site cache tag. Previously, it was only the login block that was adding that cache tag, so we were able to remove it. Not anymore. Just slightly less change!

This should actually be green… :)

Only in case this needs to be re-rolled again:

+++ b/core/modules/page_cache/src/Tests/PageCacheTagsIntegrationTest.php
@@ -95,7 +96,6 @@ function testPageCacheTags() {
-      'config:block.block.bartik_login',

+++ b/core/modules/user/src/Plugin/Menu/LoginLogoutMenuLink.php
@@ -0,0 +1,48 @@
+ * A menu link that shows "Log in" or  "Log out" as appropriate.

Double space between or and "Log

(Or maybe can be fixed on commit)

Nice to see this getting to green and code review looks good to me. See #78 for single space that can be removed on commit (I hope).

Broken PIFR testbot:

GET http://ec2-52-27-255-15.us-west-2.compute.amazonaws.com/checkout/test-ungroup-rows returned 0 (0 bytes).

It can't die soon enough.

+1 rtbc, just a nit for commit time ;)

+++ b/core/modules/user/src/Plugin/Menu/LoginLogoutMenuLink.php
@@ -0,0 +1,48 @@
+ * A menu link that shows "Log in" or  "Log out" as appropriate.

extra space before "Log out"

@WimLeers: the PIFR bot, with the URL of /ec2-52-27-255-15.us-west-2.compute.amazonaws.com, has been terminated.

Awesome :)

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -206,15 +206,13 @@ protected function menuLinkCheckAccess(MenuLinkInterface $instance) {
   -      // Use the definition here since that's a lot faster than creating a Url
   -      // object that we don't need.
   -      $definition = $instance->getPluginDefinition();
   -      // 'url' should only be populated for external links.
   -      if (!empty($definition['url']) && empty($definition['route_name'])) {
   +      $route_name = $instance->getRouteName();
   +      // When no route name is specified, this must be an external link.
   +      if (empty($route_name)) {
   ...
   -        $access_result = $this->accessManager->checkNamedRoute($definition['route_name'], $definition['route_parameters'], $this->account, TRUE);
   +        $access_result = $this->accessManager->checkNamedRoute($route_name, $instance->getRouteParameters(), $this->account, TRUE);
   
   +++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
   @@ -130,7 +130,7 @@ public function getUrlObject($title_attribute = TRUE) {
   -      return new Url($this->pluginDefinition['route_name'], $this->pluginDefinition['route_parameters'], $options);
   +      return new Url($this->getRouteName(), $this->getRouteParameters(), $options);
   

   Is there test coverage for these changes?

2. +++ b/core/modules/menu_ui/src/MenuForm.php
   @@ -358,6 +358,9 @@ protected function buildOverviewTreeForm($tree, $delta) {
   +        elseif ($id === 'menu_plugin_id:user.login.logout') {
   +          $form[$id]['title']['#suffix'] = ' (' . $this->t('<q>Log in</q> for anonymous users') . ')';
   +        }
   

   Is this an issue to make this sort of thing easier for contrib - this seems a really odd place to hard code this.

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -206,15 +206,13 @@ protected function menuLinkCheckAccess(MenuLinkInterface $instance) {
   -      // Use the definition here since that's a lot faster than creating a Url
   -      // object that we don't need.
   -      $definition = $instance->getPluginDefinition();
   -      // 'url' should only be populated for external links.
   -      if (!empty($definition['url']) && empty($definition['route_name'])) {
   +      $route_name = $instance->getRouteName();
   +      // When no route name is specified, this must be an external link.
   +      if (empty($route_name)) {
            $access_result = AccessResult::allowed();
          }
          else {
   -        $access_result = $this->accessManager->checkNamedRoute($definition['route_name'], $definition['route_parameters'], $this->account, TRUE);
   +        $access_result = $this->accessManager->checkNamedRoute($route_name, $instance->getRouteParameters(), $this->account, TRUE);
   

   I think the proper way would be: $url = $instance->getUrlObject(); $url->isRouted()

   menuLinkCheckAccessWe have a unit test here, so we should expand our test coverage, and then see whether our fix works.

2. +++ b/core/modules/menu_ui/src/MenuForm.php
   @@ -358,6 +358,9 @@ protected function buildOverviewTreeForm($tree, $delta) {
            if (!$link->isEnabled()) {
              $form[$id]['title']['#suffix'] = ' (' . $this->t('disabled') . ')';
            }
   +        elseif ($id === 'menu_plugin_id:user.login.logout') {
   +          $form[$id]['title']['#suffix'] = ' (' . $this->t('<q>Log in</q> for anonymous users') . ')';
   +        }
            elseif (($url = $link->getUrlObject()) && $url->isRouted() && $url->getRouteName() == 'user.page') {
              $form[$id]['title']['#suffix'] = ' (' . $this->t('logged in users only') . ')';
            }
   

   What about adding a new follow up to have an adminLabel() method on the menu plugin to do stuff like that?

3. +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -891,8 +891,7 @@ private function getStandardMenuLink() {
   -    $result = $menu_link_manager->loadLinksByRoute('user.logout');
   -    $instance = reset($result);
   +    $instance = $menu_link_manager->getInstance(['id' => 'user.login.logout']);
   

   Any reason for this change? Seems totally unnessary and kinda limits stuff.

4. +++ b/core/modules/user/src/Plugin/Menu/LoginLogoutMenuLink.php
   @@ -0,0 +1,48 @@
   +  public function getTitle() {
   +    if (\Drupal::currentUser()->isAuthenticated()) {
   +      return $this->t('Log out');
   ...
   +  public function getRouteName() {
   +    if (\Drupal::currentUser()->isAuthenticated()) {
   +      return 'user.logout';
   +    }
   

   I can haz injection?

5. +++ b/core/modules/user/user.links.menu.yml
   @@ -3,11 +3,10 @@ user.page:
      menu_name: account
   -user.logout:
   -  title: 'Log out'
   -  route_name: user.logout
   +user.login.logout:
      weight: 10
      menu_name: account
   +  class: Drupal\user\Plugin\Menu\LoginLogoutMenuLink
    entity.user.collection:
      title: People
   

   Does that mean we need an update path for that?

#77 Fixed the space nit :)

#87

1. Nope, fixed.
2. We discussed this in IRC:
   

   11:34:37 <WimLeers> alexpott: that is a highly specific exceptional use case
   11:35:16 <WimLeers> alexpott: because this is a menu link that changes depending on whether the user is logged in or not
   11:35:49 <WimLeers> alexpott: improving this requires infrastructure changes in the rest of the menu system
   11:36:02 <WimLeers> alexpott: I agree we wouldn't have this ideally
   11:36:38 <catch> What was the problem with having one link that shows when logged out and a different one that shows when logged in?
   11:36:39 <alexpott> WimLeers: yeah I don't think this issue should change it - more just a comment
   11:36:55 <alexpott> WimLeers: I needs worked it for the other changes that didn't seem to have explicit test coverage
   11:37:11 <WimLeers> alexpott: yep, doing that now
   11:37:27 <WimLeers> alexpott: that was a fix for something missed during the menu system revamp :/
   11:37:51 <alexpott> WimLeers: stuff get's missed all the time - Drupal is big
   11:37:55 <WimLeers> Clearly we have still big gaps in test coverage for the menu system :/
   11:37:58 <WimLeers> Yep
   11:38:23 <WimLeers> I just mean that this wouldn't have been necessary otherwise
   11:38:46 <WimLeers> catch: "link to every page"
   11:39:11 <WimLeers> catch: that permission causes the "log in" link to show up even when already logged in
   11:39:40 <WimLeers> catch: That was deemed unacceptable, even though it only affects users with that permission
   11:39:46 <WimLeers> Which makes sense IMO
   11:40:47 <catch> now I remember.
   11:41:52 <catch> It's pretty ropey that the permissions works like that for rendered menus though.
   11:42:47 <WimLeers> Yeah, agreed
   11:43:19 <WimLeers> catch: ironically, what is missing is… context!
   11:43:32 <WimLeers> Cfr. SafeMarkup :P
   

#87:

1. Works for me, done. I was just trying to stay as close to the spirit of the previous code as possible.
2. Done: #2568785: Add MenuLinkInterface::getAdminLabel()
3. Huh? This is just a specific test, and we're changing it from the old menu link plugin to the new. It was not loading by route for any reason, as the accompanying comment indicates:
   

       // Retrieve menu link id of the Log out menu link, which will always be on
       // the front page.
   

4. Done.
5. Nope, the old link will disappear, the new link will appear instead. Only if they have moved this link to a different menu or changed its weight, does this cause any change. To address that, I could alternatively choose to keep it named user.logout, then there really would be zero need for an upgrade path. That probably makes more sense. Did that.

I've also attached two fail patches: the FAIL1 patch is without hunk one (in DefaultMenuLinkTreeManipulators), the FAIL2 patch is without hunk 2 (in MenuLinkBase). They'll show the test coverage is adequate.

Huh? This is just a specific test, and we're changing it from the old menu link plugin to the new. It was not loading by route for any reason, as the accompanying comment indicates:

Well, I just think we loose a bit of test coverage when we deal with plugin IDs instead of route names.

+++ b/core/modules/user/user.links.menu.yml
@@ -4,10 +4,9 @@ user.page:
 user.logout:
-  title: 'Log out'
-  route_name: user.logout
   weight: 10
   menu_name: account
+  class: Drupal\user\Plugin\Menu\LoginLogoutMenuLink

Nice, less problems!

#2511516: Make local tasks and actions hooks provide cacheability metadata, to make the blocks showing them cacheable conflicted with this. Rebased.

Analogous FAIL patches as in #88.

Back to RTBC per #89.

I can't see how can commit this until #953034: [meta] Themes improperly check renderable arrays when determining visibility is resolved? There's a massive visual regression on the anonymous frontpage of bartik.

That issue hasn't gone anywhere in five years. :(

Bartik relying on that (see bartik_preprocess_html()) IMHO is a long-standing bug, which simply has had no incentive to get fixed because in the 99% use case, we have something in the first sidebar, both in core tests and in actual contrib usage. In 99% of cases, we don't have a second (right) sidebar.

This is the first issue in years that just happens to make this problem more apparent.

Because, in fact, this exact same problem exists in HEAD also. To reproduce:

1. Install HEAD fresh, using the Standard install profile.
2. Create an authenticated user.
3. Log in as the authenticated user.
4. Note that authenticated users by default can not add content, so the menu block is not visible.
5. Note that authenticated users by default can not search, so the "Tools" menu block is not visible.
6. Hence, this is what it looks like:
   

@Wim Leers yes fine but with this patch we expose it to everyone all of the time.

Of course.

I'll ping some Bartik people and get their thoughts. Perhaps they see an easier way forward.

Looking at the issue summary this patch was primarily about bartik in a mobile environment. Bartik's design is very sparse and removing the login block emphasizes that on larger screen formats while making it look less clunky on mobile. But that is Bartik's design and custom themes won't be affected.
A drop down login block was suggested but that would still leave a largely empty screen for anonymous users. Or a much trimmer, leaner login block could be designed.
Bartik has the same front page content as Stark, just more visual. This doesn't look like a visual regression to me, it just emphasizes that bartik is stark.

From usability point of view you wouldn't want text that spans the whole width of the page. I think keeping the main content column this narrow is a good thing. I'm concerned that Bartiks "flexi layout" implementation is effectively broken but from reading the comments it sounds like this is already the case.

Assigning to Emma for the final decision.

I agree with removing the user login block and having the login link in the same place.

I also agree with #110 that the main content should not span full width. However that is not possible right now with the layout-one-sidebar logic that is kicking in because of the empty sidebar printing.

This issue now makes Bartik look sparse with content which is fine but I do not agree with the sidebar wrapper printing empty and forcing all main content to the right.
If you add real content, the gap on the left caused by the sidebar is very obvious and it looks like a bug. I have shown the screenshot to a few others and they have winced at it too.
 

 
I know we have this issue that tries to fix the cause of the sidebar printing #953034: [meta] Themes improperly check renderable arrays when determining visibility but it looks like there will not be a solution anytime soon.

#112: Lets open a dedicated bartik issue. While placeholders are a reality now as access is checked and cached separately we should be able to fix that particular bartik problem easily.

Actually there is an issue already:

#2529308: bartik_preprocess_html() only works correctly without lazy rendering, without placeholders, without cacheability metadata for access checks

Lets discuss that bug over there.

Here's a straight rebase of #93. #93 no longer applied cleanly.

I rerolled #2529308: bartik_preprocess_html() only works correctly without lazy rendering, without placeholders, without cacheability metadata for access checks, that should be able to go in now.

Except that that patch does NOT actually solve the problem. Because of the reasons already cited in #97, which I'll repeat here:

Because, in fact, this exact same problem exists in HEAD also. To reproduce:

1. Install HEAD fresh, using the Standard install profile.
2. Create an authenticated user.
3. Log in as the authenticated user.
4. Note that authenticated users by default can not add content, so the menu block is not visible.
5. Note that authenticated users by default can not search, so the "Tools" menu block is not visible.
6. Hence, this is what it looks like:
   

But I think I can make it more clear by showing a screenshot of the everything is in at the time where bartik_preprocess_html() runs, and thus at which time it tries to decide to add either the layout-two-sidebars class, or layout-one-sidebar, or layout-no-sidebars. Here you go:

As you can tell, the structure looks like this:

$variables['page']['sidebar_first'] = [
  'bartik_tools' => [
    '#cache' => …,
    '#weight' => …,
    '#lazy_builder' => …,
  ],
  '#sorted' => …,
  '#theme_wrappers' => …,
  '#region' => 'sidebar_first',
];

1. So, we see that the bartik_tools block is present. Because any user can in fact access that block. It's just that that block may turn out to be empty in the end, if the user does not have access to any of the links in that block.
2. Therefore, because that block is rendered lazily, it is impossible to know whether this region will be empty.
3. Similarly, because that block may be automatically placeholdered (if we already know it varies by user or by session, so that its poor cacheability does not make the entire page uncacheable) it is totally possible that that block does end up rendering to the empty string.
4. And finally, to tie this all together in what is hopefully the clearest possible example: in a world where we do BigPipe-style rendering (see https://www.youtube.com/watch?v=JwzX0Qv6u3A), or in general, any rendering where we the things that are cheap to render before the things that are expensive to render, it is effectively impossible for the page template to know which regions will be empty and which ones will not be empty. We need layout to be predictable regardless of whether certain blocks render to the empty string or not. Imagine that we assume the first sidebar is empty, and therefore apply the layout-no-sidebars class. Then, a block is delivered, and the first sidebar is no longer empty, so we then suddenly apply the layout-one-sidebar class. That would cause massive jumping of content. We don't want massive jumping of content, therefore we don't want the behavior that bartik_preprocess_html() currently has.

In other words: bartik_preprocess_html()'s conditional page-level classes do not make sense at all in a world where you want things to be fast, because it requires you to always render everything on the page before assembling the page, which inherently implies very poor cacheability, and rules out lazy rendering.

Great reply, Wim. Note that custom themes are welcome to make assumptions about empty regions and such, but core themes need to always lay them out as per #117

How about we fix "bartik_preprocess_html()'s conditional page-level classes" in a separate issue? As Wim has shown, this is a pre-existing condition.

AFAIK we won't be changing the Standard install profile after RC1.

So if this doesn't land before RC1, 8.0.0 will ship with this antiquated look that's mobile-unfriendly to boot (see @webchick's research in #56), as well as lesser performance.

Given that this is RTBC again, and I don't want this to frustrate people, here are some thoughts that I think will address the concerns here and therefore make this completely independent of #2529308: bartik_preprocess_html() only works correctly without lazy rendering, without placeholders, without cacheability metadata for access checks (which is a very thorny issue that we probably don't want to resolve before RC).

I think we could easily solve the concerns raised about this being ugly by … having more sane permissions and blocks in the Standard install profile:

1. Grant the Use search permission to all users. The "Search" block is already placed in the left sidebar by default, so that would make that region never empty.
2. Adding the "Recent content" block to the left sidebar. It's kinda silly that we now have Views in core, but aren't really showing that off.

Both of those address very common needs and can be found on many (if not most) sites.

We could make either or both of those additions to the Standard install profile in this issue (if people are +1) or a separate issue (if people want to discuss it more).

Wrong tag, sorry for the noise.

@WimLeers I think we can change the install profile for "new" installs just not for existing ones

I am definitely +1 on making search enabled by default it shows of that we have a CMS, with more advanced capabilities. I am not 100% sure about recent content block as that will show up empty at first, leaving it up to Emma to decide there.

I think we can change the install profile for "new" installs just not for existing ones

That is exactly what this does :)

I think we could easily solve the concerns raised about this being ugly by … having more sane permissions and blocks in the Standard install profile:

* Grant the Use search permission to all users. The "Search" block is already placed in the left sidebar by default, so that would make that region never empty.
* Adding the "Recent content" block to the left sidebar. It's kinda silly that we now have Views in core, but aren't really showing that off.

I am +1 to these blocks, but does it make sense to do it in a follow up if it guarantees it will not land before RC? If we only have enough time to commit this one issue, the follow up becomes pointless. I think it makes more sense to make these changes here.

Yep, totally makes sense to me too :)

I'll do only the Search block/permissions, because the "Recent content" one perhaps merits further discussion because of what Bojhan said. I think the Search block one is zero risk, so let's do that one here first. We can still do "Recent content" in a follow-up if we feel strongly about it.

So, after a fresh install, this is what you get to see with #126:

(i.e. things don't look strange anymore)

Looks great, given that this is tied to the RC deadline I'd rather get this in sooner than later. We can always add one more block.

Excellent. RTBC++

RTBC + 1

Maybe we can add some sample content (release deadline polishing) to show off some of standard profile's capabilities.

Do we have a follow-up already?

RTBC++++

Also along with @FabianX would like to know where to follow the follow up issue.

4x RTBC on this—I was just thinking earlier today that other account junk is stowed in the top right corner, so why not the log in link. I came to this issue just now and noticed the latest screenshot is exactly what I pictured, and makes the default Bartik header layout consistent logged in/logged out out of the box. So effective, so simple a change, it works great.

Yep; seems like an elegant solution to the problem.

FWIW, we should be able to change Standard profile in any minor release; it should only affect new installs, not existing.

Committed and pushed to 8.0.x. Thanks!

Glad to read that, @geerlingguy. :)

Thanks all, this was a tough one!

Please revert. Breaks sqlite and pgsql tests in HEAD, as found in #2580485: Test fails on sqlite and pgsql ToolbarAdminMenuTest::testLocaleTranslationSubtreesHashCacheClear

I found the root cause.

There are two.

On MySQL, ToolbarAdminMenuTest::testLocaleTranslationSubtreesHashCacheClear(), where it saves a translation ($this->drupalPostForm('admin/config/regional/translate', $edit, t('Save translations'));) is posting the translation for Menus to the first text area on this page:

On SQLite, it looks like this:

On SQLite, but without this patch's DefaultMenuLinkTreeManipulators and MenuLinkBase hunks, it looks like this:

Root cause one: config_translation behaves differently in SQLite/Postgres vs. Mysql

Note how there is one match in MySQL versus 3 in SQLite. Note how MySQL appears to be doing case-sensitive matching, and SQLite case-insensitive.

Root cause two: different ordering with this patch

Somehow, the changes in DefaultMenuLinkTreeManipulators and/or MenuLinkBase are causing a different ordering of those 3 matches.

Conclusion

The test assumes there will be only one match. It's only by chance that HEAD is passing, because in HEAD the first match just happens to be the right one. It should actually only show one match.
So the test translates the WRONG string, and therefore the toolbar menu is the same before and after, and hence the subtrees hash doesn't change on SQLite/Postgres.

Clearly, there are much deeper problems at play here.

This was one giant WTF! It'd be funny if it wasn't just before RC and I now have to somehow fix SQLite support in code totally unrelated to this issue…

Looping in the multilingual team.

The search is supposed to be case sensitive, as noted in the help text. That it is not on SQLite/PgSQL is indeed a bug. It is not really "by chance" that "Menus" should be the only result, if the search is case sensitive, there is no other result that would be shown.

Looking at the locale schema (the bug has nothing to do with config translation):

function locale_schema() {
  $schema['locales_source'] = array(
    'description' => 'List of English source strings.',
    'fields' => array(
      // ...
      'source' => array(
        'type' => 'text',
        'mysql_type' => 'blob',
        'not null' => TRUE,
        'description' => 'The original string in English.',
      ),
      // ...
    ),
    'primary key' => array('lid'),
    'indexes' => array(
      'source_context' => array(array('source', 30), 'context'),
    ),
  );

  $schema['locales_target'] = array(
    'description' => 'Stores translated versions of strings.',
    'fields' => array(
      // ...
      'translation' => array(
        'type' => 'text',
        'mysql_type' => 'blob',
        'not null' => TRUE,
        'description' => 'Translation string value in this language.',
      ),
      // ...
  );

So in both cases, specifies blob only for MySQL specifically. I don't know if there was a reason to not specify blob for other DB engines given the 'type' key itself supports blob as per our docs:

 *     - 'type': The generic datatype: 'char', 'varchar', 'text', 'blob', 'int',
 *       'float', 'numeric', or 'serial'. Most types just map to the according
 *       database engine specific datatypes. Use 'serial' for auto incrementing
 *       fields. This will expand to 'INT auto_increment' on MySQL.
 *       A special 'varchar_ascii' type is also available for limiting machine
 *       name field to US ASCII characters.
 *     - 'mysql_type', 'pgsql_type', 'sqlite_type', etc.: If you need to
 *       use a record type not included in the officially supported list
 *       of types above, you can specify a type for each database
 *       backend. In this case, you can leave out the type parameter,
 *       but be advised that your schema will fail to load on backends that
 *       do not have a type specified. A possible solution can be to
 *       use the "text" type as a fallback.

As per our database.api.php. OTOH looking at the SQLite schema driver, it would only make a text field case insensitive if we did explictly provide a binary key with a FALSE value:

      if (in_array($spec['sqlite_type'], array('VARCHAR', 'TEXT'))) {
        if (isset($spec['length'])) {
          $sql .= '(' . $spec['length'] . ')';
        }

        if (isset($spec['binary']) && $spec['binary'] === FALSE) {
          $sql .= ' COLLATE NOCASE_UTF8';
        }
      }

Did not find the relevant code in the PgSQL driver.

#143:

It is not really "by chance" that "Menus" should be the only result, if the search is case sensitive, there is no other result that would be shown.

Of course not!

But the thing is that in HEAD with SQLite we have three matches, and so it's only by chance that the first one happens to be the one we want, which is why this problem went unnoticed until now. That's what I was saying:

It's only by chance that HEAD is passing, because in HEAD the first match just happens to be the right one.

I don't think we need to fix the locale schema here BTW, we can translate a different menu item that also shows up in toolbar but is more unique in the search or we can make the UI test more specific in which textarea it will enter the translation in and then resolve the locale schema in its own issue (with its own test). It would definitely mix too much responsibility in here if we'd need to fix that here too.

Discussed the strategy for this issue with Gábor Hojtsy and webchick. The problems on SQLite/Postgres are not introduced by this patch, but are pre-existing problems that this patch merely exposed. As #141 demonstrated.

Therefore, we decided to move the fixing of those pre-existing to a separate issue: #2580671: Locale DB schema case insensitive (blob) only on MySQL not on other databases.

Interdiff relative to #126. Simply choosing a string that doesn't have multiple matches in locale.module's search fixes the problem on SQLite/Postgres — again, proper solution of the underlying problem is for #2580671.

And, of course, also testing against SQLite and Postgres.

Fix looks like in scope on this issue, RTBC pending green test feedback :)

Nice work. Take two!

Committed and pushed to 8.0.x. Thanks!

Grrrrowl.

/me sends a llama stampede to spit the PIFR bots into oblivion.

Can we get the CR published, because #2581139: Login block is missing from the standard install?

Did we forget to put a destination=[current.path] on the login link? Without that, the link dumps you on the profile after signing in. Thats not nice IMO. We can append the querystring param via javascript in order to not bother the static and dynamic page caches.

@moshe weitzman opened #2582797: [Regression] login link has no destination=drupalSettings.path, so dumps you on the profile to look into that. needs steps to reproduce.