Add a dedicated @LanguageNegotiation annotation class

Needed a re-roll.

Missed a file on the re-roll. Let's try again.

Creating a new patch based on https://drupal.org/node/2195573#comment-8837015 adding suggestion in https://drupal.org/node/2195573#comment-8837691. Rerolling.

Re-roll.

Seeing as my contribution was limited to a fairly simple re-roll, I am RTBC'ing this.

Tagging, needs reroll.

And, ironically, I have to re-roll again. There were no conflicts that needed to be resolved manually.

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Thanks @Les Lim for adding that; it definitely helps put the change in context.

Note that "improves developer experience" does not make something a prioritized change. And there is a BC break, albeit a small one.

However, the fact that it adds documentation is valuable for all the reasons that we started #1966246: [meta] Introduce specific annotations for each plugin type in the first place. And most of the similar issues were completed before the beta. So I think it might be worth granting an exception to this and the other outstanding child issues.

However, setting this postponed for the moment to discuss it more in #1966246: [meta] Introduce specific annotations for each plugin type (where I'll post a comment shortly).

Unpostponing following the discussion on #1966246: [meta] Introduce specific annotations for each plugin type -- thanks for your patience!

Note: I am restoring the previous status; I have not reviewed this issue and so am not marking it RTBC myself. :)

Ah, but we need to add/update a change record. We can create one for the whole meta and just mention these two particular issues as being recently changed.

1. +++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
   @@ -0,0 +1,86 @@
   + * For a working example, see \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationBrowser
   

   Should be on two lines and end with a fullstop.

2. +++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
   @@ -0,0 +1,86 @@
   +  /**
   +   * An array of allowed language types. If a language negotiation plugin does
   +   * not specify which language types it should be used with, it will be
   +   * available for all the configurable language types.
   +   *
   +   * @var array
   +   */
   +  public $types;
   

   Should have a one line summary. The first sentence looks good.

3. +++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
   @@ -0,0 +1,86 @@
   +  /**
   +   * The value Drupal's page cache should be set to for the negotiation plugin
   +   * to be invoked. May be omitted if the page cache status is not relevant to
   +   * the plugin.
   +   *
   +   * @var int
   +   */
   +  public $cache;
   

   I can't spot where this is being used. Also I do not understand the documentation. Plus each property should have a one line summary.

1. Should be on two lines and end with a fullstop.

   Done.

2. Should have a one line summary. The first sentence looks good.

   Done.

3. I can't spot where this is being used. Also I do not understand the documentation. Plus each property should have a one line summary.

   I guess it was used previously but I can't find any usage now either. Removed.

Just one minor thing. Maybe it's just me...

1. +++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
   @@ -0,0 +1,80 @@
   +   * The default weight of the negotiation plugin.
   

   this

2. +++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
   @@ -0,0 +1,80 @@
   +   * The human-readable name of the language negotiation plugin.
   

   ...and this

3. +++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
   @@ -0,0 +1,80 @@
   +   * The route pointing to the plugin's configuration page.
   

   ..and this

...all describe the same "thing", only with different words.

I realize that the last one could be hard to fit in one line using "language negotiation plugin". But could we at least try this?

diff --git a/core/modules/language/src/Annotation/LanguageNegotiation.php b/core/modules/language/src/Annotation/LanguageNegotiation.php
index 1365854..cc802df 100644
--- a/core/modules/language/src/Annotation/LanguageNegotiation.php
+++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
@@ -46,7 +46,7 @@ class LanguageNegotiation extends Plugin {
   public $types;
 
   /**
-   * The default weight of the negotiation plugin.
+   * The default weight of the language negotiation plugin.
    *
    * @var float
    */

Other than that, looks great. Since the change is so small, here is a patch and I am setting it to RTBC.

I just added one word in a comment. :)

Thanks @eiriksm, I'm okay with the RTBC in that case too. ;) Also thanks for the change record; I think that works.

Two more points of feedback:

1. +++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
   @@ -0,0 +1,80 @@
   * @var float
   

   Is this really a float and not an int? Edit: the weight property; sorry about the missing context in my diff snippet.

2. +++ b/core/modules/language/src/Annotation/LanguageNegotiation.php
   @@ -0,0 +1,80 @@
   +   * An array of allowed language types.
   +   *
   +   * If a language negotiation plugin does not specify which language types it
   +   * should be used with, it will be available for all the configurable
   +   * language types.
   +   *
   +   * @var array
   +   */
   +  public $types;
   +   

   Shouldn't this specify that it's an array of \Drupal\Core\Language\LanguageInterface[]?

   (Double-check that; don't just change it blindly.) :)

Setting NR (rather than NW) because I'm not 100% sure of either of the above; they should be confirmed. Thanks!

Edit: Fixed some dreditor weirdness.

Shouldn't this specify that it's an array of \Drupal\Core\Language\LanguageInterface[]?

Regardless of what the variable contains, @var array is never really specific enough to be useful.

Here's a re-roll, will make a few other improvements in another patch.

An unexpected conference call made this take a little longer than expected.

I confirmed weights are integers by looking at the Language module's configuration schema file. I will open a follow-up for related type errors elsewhere.

Shouldn't this specify that it's an array of \Drupal\Core\Language\LanguageInterface[]?

Almost. The array consists of string values of constants on that specific interface. I extended the documentation for this as well. Your comment was a nice push in the right direction, though. Thank you!

Actually, language types are pluggable as well. See hook_language_types_info(). We can't document the annotation property as being an array of interface constants only.

Patch still does the trick.

The changes done since last rtbc also looks sane to me. RTBC from me.

Back to rtbc

Hm, this is RTBC still as far as I can tell.

+++ b/core/lib/Drupal/Core/Field/WidgetBaseInterface.php
@@ -62,8 +62,8 @@ public function extractFormValues(FieldItemListInterface $items, array $form, Fo
-   * @param \Symfony\Component\Validator\ConstraintViolationListInterface $violations
-   *   A list of constraint violations to flag.
+   * @param \Symfony\Component\Validator\ConstraintViolationListInterface|\Symfony\Component\Validator\ConstraintViolationInterface[] $violations
+   *   The constraint violations that were detected.

This looks unrelated? Is it?

You are right, it is. Probably snuck in for @Xano, as it is not in the interdiff (which was probably why I did not catch it while reviewing).

Updated patch.

Sorry. Bad file. Here is the patch

I also noticed that the patch in #34 contained changes to "core/modules/system/src/Form/ModulesListForm.php" which were not in the first patches in this issue. Also seems unrelated. Here is a patch that is based on #31 with what was probably the intended changes in #34.

Manually reviewed the code. The last patch applies cleanly to current head.

Committed 2f370af and pushed to 8.0.x. Thanks!

Thansk for completing the beta evaluation.

Thanks all.