1. Attached patch clarifies on the real DX problems that everyone happens to ignore.
  2. Comments are intentionally written emotional and borderline offending.
  3. ???????
  4. Profit.
CommentFileSizeAuthor
dx1.0.patch24.15 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Would be great to get some individual issues open to discuss some of these. While some are truly valid, I'm sure there are historic reason for others that are easily fixed.

Glad we have some direction.

thedavidmeister’s picture

Status: Active » Needs work

+ #

Trailing white space.

thedavidmeister’s picture

@sun, maybe join the discussion/efforts at #2093143: [meta] Remove calls to @deprecated and "backwards compatibility" procedural functions from core. It might help address some part of your DX concerns.

dawehner’s picture

jibran’s picture

Other then the in progress route conversions I think rest of the points are valid. IMHO we honestly need some discussion to figure out some middle way. Like String:: point, it is like this because of unit tests but I am sure we can figure something out.

RobLoach’s picture

Like String:: point, it is like this because of unit tests but I am sure we can figure something out.

    * @var array
    *
    * @see \Drupal\Component\Utility\String::filterXssAdmin()
+   *                                   ^^ ^^ ^^ All wrong.
    */

If it's the above you're referring to, it's just simply a typo. Should be \Drupal\Component\Utility\Xss::filterAdmin(). If we open up an issue for it, we'll be able to patch them individually.

EclipseGc’s picture

So, I'll see if I can justify a bit of this. (not all by any stretch, but just a few things)

1.) Use statements don't import anything, they just make a class available to be instantiated. Since we type hint on the short names, full use statements are used. This makes method signatures not look crazy, but does mean in some instances you end up with quite a few use statements.

2.) {@inheritdoc} can, and usually DOES, imply that documentation exists in an interface that method came from, not that you're overriding an existing method, though both cases are valid (I think).

3.) We had a long drawn out debate on the name "controller" in relation to entity stuff. I don't recall the actual resolution to that, but suffice it to say, we're not blind to all of these consistency issues. People are discussing changes and trying to fix it.

4.) You seem really hung up on the use statements. I'm not sure why this is. Perhaps you could elaborate about that topic a bit more and why it specifically bothers you? I'm happy to be helpful if possible.

5.) Your criticisms of route definition are pretty fair imo. I think there's a lot of dissatisfaction there, which is unfortunate, but we discussed this at length in Prague on numerous occasions. You're not alone, people are discussing and experimenting with some potential solutions.

All in all, I don't think I find any of this really surprising. I wish we'd had your voice to add to the discussion in Prague. I too, hope we can solve many of the issues you've pointed out. People ARE working on a lot of this stuff though.

Eclipse

damiankloip’s picture

+++ b/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php
@@ -49,48 +52,56 @@ class NodeRevisionAccessCheck implements AccessCheckInterface {
+    # FWIW, you forgot to return static::ALLOW and static::DENY and static::FUCKTHAT here.

Nope, This is just a method to check if the access checker applies to the route; NOT to check access.

thedavidmeister’s picture

Title: [meta] DX of Doom™, Part I » [meta] Prioritise changing the parts of Drupal core that @sun personally finds most distasteful as at 29 Sep 2013
Priority: Critical » Normal
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +Needs issue summary update

It's nearly impossible to do or say anything constructive here as-is, and it looks like @sun put little to no effort into presenting his complaints in a constructive format, which is a real shame (and more offensive than the comments themselves). The current issue summary and attached patch aren't funny, at all, and it's unclear what it aims to accomplish, beyond propagating a general sense of disappointment.

  • The issue summary is demonstrably wrong, it claims that "everyone happens to ignore" issues that are, in fact, being actively worked on. At most, the issue summary can only accurately claim the visibility of, and progress made in, these discussions is too low (which is probably true, but not worth opening a critical meta to complain about).
  • This issue is listed as [meta], but references no other issues, lists no tasks, doesn't explain what issues should be opened and has a patch attached that goes out of its way to be offensive.
  • As well as being deliberately offensive, the patch attached is little more than a disjointed rant disguised as something technical - can we please close this thread as "generally unconstructive", or immediately split it out into sub-issues with defined (or at least definable) scope.
  • As pointed out in #6, the patch throws together examples that are clearly minor typos, easily fixed, with problems that will require extended discussion and prototyping to fix - The patch is equal parts emotional rhetoric and "real" complaints, making it needlessly hard for others to interpret or provide feedback for.

Essentially, this issue in its current format is just trolling the community and wasting people's valuable time.

My preference would be that, if we're to continue doing anything at all here, we first:

- halt any further discussion until each part of the patch attached to the issue has been converted into a reference to a new/existing issue or actionable task in the summary so that we're all at least talking about the same thing(s).
- Identify a common underlying problem with all the comments in the patch in the summary and rewrite the issue title and summary so it's clear how they're all related beyond "critical-ness", "Drupal", "Sun" and "DX", comment #7 demonstrates that there's at least 5 independent complaints being raised here that are each different enough that it has become hard to see how they could all be "in scope" for a single meta discussion
- Identify which complaints in the patch are truly "critical" and which parts are really just a little bit inconvenient and were "swept up" into the general rant

Crell’s picture

Status: Postponed (maintainer needs more info) » Closed (won't fix)
Issue tags: -Needs issue summary update

This issue is not useful or actionable.

larowlan’s picture

Title: [meta] Prioritise changing the parts of Drupal core that @sun personally finds most distasteful as at 29 Sep 2013 » [meta] Ensure that DX issues identified by a recent review are covered with individual issues
Status: Closed (won't fix) » Closed (duplicate)

I think we need to at least ensure that the issues identified are covered in new or existing issues and are listed on the DX intiative page (https://drupal.org/node/2081777)
There are some minor documentation bugs that I have filed.
There are also some valid DX concerns, although we all agree that the delivery method doesn't help.
However I can accept that frustration is often expressed as sarcasm when you genuinely care so let's just let this go through to the keeper.
Created so far:

Already existed:

I think that covers most of it so updating status.
Have added #2104347: Unify argument order between \Drupal\Core\Entity\EntityAccessControllerInterface access and createAccess methods to the DX initiative page

jibran’s picture

Thank you @larowlan for the great work and sane reply.

rafamd’s picture

^^^ indeed