Updated: Comment 0

Problem/Motivation

If you first time teach people about routing/local tasks and finally .services.yml files you will get the result that it is harder for people to
understand why they need to specify services: yml file in these files, even the filename already specify it.

Proposed resolution

Allow to specify services on the root level.
Before:

services:
  access_check.permission:
    class: Drupal\user\Access\PermissionAccessCheck
    tags:
      - { name: access_check }
  access_check.user.register:
    class: Drupal\user\Access\RegisterAccessCheck
    tags:
      - { name: access_check }

after

access_check.permission:
  class: Drupal\user\Access\PermissionAccessCheck
  tags:
    - { name: access_check }
access_check.user.register:
  class: Drupal\user\Access\RegisterAccessCheck
  tags:
    - { name: access_check }

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#23 2139583-23.patch956 bytesdawehner
#13 2139583-13.patch110.99 KBdamiankloip
#11 2139583-11.patch111.14 KBdamiankloip
#1 services-2139583.patch4 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
services-2139583.patch4 KB

This is just a prototype to see whether anything fails hard.

Status: Needs review » Needs work

The last submitted patch, 1: services-2139583.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

1: services-2139583.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: services-2139583.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

1: services-2139583.patch queued for re-testing.

Crell’s picture

Crell CreditAttribution: Crell commented

I'm a little bit leery, as this introduces a potential incompatibility with the same file in Symfony; without this patch, the syntax we're using is identical to Symfony's.

OTOH, Symfony fullstack for whatever strange reason tends to use XML files instead of YAML for the services file, so... I'm torn.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Priority: Normal » Critical

Everyone who writes a service file by hand will potentially run into this problem, so I consider this critical.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

YES experienced thuis several times already smashing my head against the desk. If we do this we should do it before beta.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +beta blocker

Either needs to block beta or it's not critical, went for beta blocker.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Critical » Major
Issue tags: -beta blocker +beta target

Actually no, seems good but it's not the end of the world.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2139583-11.patch111.14 KB

Rerolled and converted all core services.yml files

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Created a drafted change notice: https://drupal.org/node/2197139

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2139583-13.patch110.99 KB

Reroll.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

13: 2139583-13.patch queued for re-testing.

Crell’s picture

Crell CreditAttribution: Crell commented

Actually I just started working on a demo module for a class where using a parameter for something makes total sense. Let's not sell parameters short.

Also, this would make those files less consistent and more divergent from existing Symfony documentation we don't want to rewrite. I'm overall -1. Really, I've yet to be bothered by the "services" header.

Crell’s picture

Crell CreditAttribution: Crell commented

Another thing that just occurred to me: It means if I have a file that has just services with no header, and I want to add a parameter, what would have been a 3 line patch is now a "whole file replace" patch.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Meh, that's kind of a moot point IMO :) You add one line and then tab the existing YAML in the file to indent it. Yes, the diff will be bigger - Who really cares about that though.

Let's also remember, people using parameters on the container in services.yml files is going to be a rarity, rather than the norm.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Also, this would make those files less consistent and more divergent from existing Symfony documentation we don't want to rewrite. I'm overall -1. Really, I've yet to be bothered by the "services" header.

So? We don't support bundles nor does bundles support modules. Also note this is optional. I would be fine with no changing core at all, but at least remove that sillyness for people when they write custom modules.

Another thing that just occurred to me: It means if I have a file that has just services with no header, and I want to add a parameter, what would have been a 3 line patch is now a "whole file replace" patch.

This is not true. It will just work as always. You can still indent and put parameters under the parameters key.

Let's also remember, people using parameters on the container in services.yml files is going to be a rarity, rather than the norm.

Even I don't like that, this statement is probably true.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Also, this would make those files less consistent and more divergent from existing Symfony documentation we don't want to rewrite. I'm overall -1. Really, I've yet to be bothered by the "services" header.

Well, this is optional, noone has to. It is just nicer. Symfony bundles are so different to modules anyway.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Yeah, I think this is nicer, plus it is optional. If we have to, we can not convert all of the core services files?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I agree with @Crell here.

But instead of just mulling over hypothetical consistency concerns, I disagree with this proposal, because I want to kill the inconsistency, in particular to improve DX:

#2251113: Use container parameters instead of settings

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well, in those case we maybe should rename the file to services_parameters.yml

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
2139583-23.patch956 bytes

AT least we should improve the really unhelpful exception message.

Status: Needs review » Needs work

The last submitted patch, 23: 2139583-23.patch, failed testing.

Crell’s picture

Crell CreditAttribution: Crell commented

I have no idea why testbot doesn't like #23, but I like that approach combined with #2251113: Use container parameters instead of settings a lot better.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review

23: 2139583-23.patch queued for re-testing.

Just checking whether that failure is for reals.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Well #23 is definitely an improvement over HEAD. But do we really want to cannibalize this issue with that? What about opening a small dedicated issue for that, and leaving this one in case we decide to go with that?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#23 makes sense. Though the same fix should be filed as an upstream PR in tandem, because the only reason for why this code even exists as a literal copy is that the YamlFileLoader methods are declared as private upstream (which should be a separate PR that I wanted to create last weekend but forgot).

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I don't think we should wait at all on upstream because the code is totally different. They support arbitrary key via their extensions.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Move the parameters out of the .services.yml file » Clarify error message when services.yml definitions are missing top-level keys
Status: Needs review » Reviewed & tested by the community

Yeah, I didn't mean to "wait."

FYI, created https://github.com/symfony/symfony/pull/10920 for the method visibility problem, so that we can properly extend this class hopefully soon.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

That PR didn't work out, so instead I created:

#2269385: DependencyInjection YamlFileLoader is out of date

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

That error message doesn't seem like a major task...

Committed/pushed to 8.x, thanks!

  • Commit 3b9abe1 on 8.x by catch: 
    Issue #2139583 by damiankloip, dawehner: Clarify error message when...

Status: Fixed » Closed (fixed)

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