Create helper methods for "Add paragraphs" modes

Let's see how much code we can drop if we remove this.

I'm highly open to user inputs if this is really important to someone. But yeah, code complexity sucks to maintain 4 different ways of adding paragraphs... especially if we add the widget soon at #2236905: [META] Nicer UI / Icons for "Paragraph type" + "Add another Paragraph"

Yeah, and the tests...

But for the moment that's not making a big difference, thus cancelling this experiment.

What we should do though, is outsource these things into helper methods, say buildAddButton() / buildAddButtonSelect().
We also had the idea to build helper methods for the widget that are based on the widget/edit mode.
This is an important step since we need to find a way to put the spaghetti more into component logic, since our master plan will add even more modes and logic.

I will try this one.

Extract code for creating add_more button into new buildAddButtonSelect method.

Some quick reply.

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -877,7 +877,67 @@ class InlineParagraphsWidget extends WidgetBase {
   +      $real_item_count,
   +      $elements,
   +      $access_options,
   +      $title,
   +      $id_prefix,
   +      $parents,
   +      $field_name,
   +      $wrapper_id
   

   That reads much unfortunate... I think we need a more specific battleplan about how we can outsource this, and if at all.
   Should we put more to the form state and work with these elements? Or use some other helpers to get these values?
   Not sure of we are allowed to put anything into a member variable?

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -877,7 +877,67 @@ class InlineParagraphsWidget extends WidgetBase {
   +  public function form(FieldItemListInterface $items, array &$form, FormStateInterface $form_state, $get_delta = NULL) {
   
   @@ -953,46 +1014,30 @@ class InlineParagraphsWidget extends WidgetBase {
   -  public function form(FieldItemListInterface $items, array &$form, FormStateInterface $form_state, $get_delta = NULL) {
   

   Please don't move this method.

This can be improved a lot by moving some more code into the helper method and changing some things so that we don't insert into $elements anymore but just return the inner element. Means we need to keep the primary if in there or we need to handle returning nothing. I think that's OK like that.

Some more refactoring would be possible now, e.g. using early returns for the special cases instead of many nested if/else conditions.

$items and $form_state can be avoided by moving the translation init out of the helper and making it part of the if condition instead of using #access.

Not sure about $parents, $id_prefix, $wrapper_id. Could use object properties to pass them around but the amount of variables is probably acceptable now.

Similar for $real_items_count, that is fun anyway, if I see that correctly, we we drop paragraphs that you don't have access to when editing? seems like we'd need some kind of read-only mode there and then this might also become simpler.

Also needs documentation and maybe a bette rmethod name, I just took the existing one.

Made two new methods buildDropDown and buildList. Also extract some parameters as class property. I think that we need to rename new properties to something similar because we have local variables in formElement, and that could be very confusing.

Added one new helper method getAccessOptions(). Only need to figure out how to remove add_more_elements from parameters.
Also changed syntax for array.

Now is looking fine, ready for review.

Some quick responses for improvements. :-)
Cross-post, but don't have time to cross-check with john, sorry

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -933,6 +857,165 @@ class InlineParagraphsWidget extends WidgetBase {
   +      return $this->buildDropDown();
   ...
   +      $add_more_elements = $this->buildList();
   ...
   +    return $add_more_elements ;
   

   just
   return $this->buildList()
   Nothing more below.

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -933,6 +857,165 @@ class InlineParagraphsWidget extends WidgetBase {
   +  public function getAccessOptions() {
   

   OK the previous variable name was access_options, but we should seek a better name for a method.
   getAllowedTypesWithAccess() or so.

3. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -933,6 +857,165 @@ class InlineParagraphsWidget extends WidgetBase {
   +    $dragdrop_settings = $this->getSelectionHandlerSetting('target_bundles_drag_drop');
   ...
   +      if ($dragdrop_settings || (!count($this->getSelectionHandlerSetting('target_bundles'))
   ...
   +          $this->access_options[$machine_name] = $bundle['label'];
   

   That's strange. Just because those dragdrop_settings are there, it's a pass.
   Instead, we should check the drag_drop settings, if the type is enabled.
   The settings would otherwise be dysfunctional.
   However, this logic doesn't change, so let's cover this problem in a follow-up and investigate more!

4. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -933,6 +857,165 @@ class InlineParagraphsWidget extends WidgetBase {
   +  public function buildDropDown() {
   

   What DropDown?

5. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -933,6 +857,165 @@ class InlineParagraphsWidget extends WidgetBase {
   +  public function buildList() {
   

   What list?

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -691,20 +726,20 @@ class InlineParagraphsWidget extends WidgetBase {
+    static::setWidgetState($this->parents, $field_name, $form_state, $field_state);

@@ -748,33 +783,11 @@ class InlineParagraphsWidget extends WidgetBase {
+    static::setWidgetState($this->parents, $field_name, $form_state, $field_state);

I think one setWidgetState call would be enough?

@yongt9412 :
#16.4 @Berdit said that can stay like this for now.
#16.6 I don't think we need boolean argument here. That will not change logic a lot and I think is more readable, because the name of function is not clearly saying what should be parameter.

@miro_dietiker
#18 I think that we can not use only one, test are failing.
Created follow-up: #2834362: Change logic in getAllowedTypesWithAccess

Mode instead of Model.

@yongt9412 :
As I was thought, we can not use if (count($this->getAllowedTypes())) { instead of if (count($this->getAllowedTypes()) == 0) { .

Both lines inverted the logic.

@miro_dietiker yes, I did that after discussing with @Berdir. We are checking at the beginning of function and in that way we avoid else statement.
I forgot to switch other properties to be camelCase.

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -43,6 +43,41 @@ class InlineParagraphsWidget extends WidgetBase {
      /**
   +   * Prefix id.
   +   *
   +   * @var string
   +   */
   +  protected $fieldIdPrefix;
   +
   +  /**
   +   * Full id.
   +   *
   +   * @var string
   +   */
   +  protected $fieldWrapperId;
   

   Those descriptions don't really tell me anything. also id should be ID

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -43,6 +43,41 @@ class InlineParagraphsWidget extends WidgetBase {
   +   * @var integer
   +   */
   +  protected $realItemCount;
   

   int, not integer

3. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -43,6 +43,41 @@ class InlineParagraphsWidget extends WidgetBase {
   +  /**
   +   * Parents for the current paragraph.
   +   *
   +   * @var array
   +   */
   +  protected $parents;
   

   why no field prefix here?

4. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -43,6 +43,41 @@ class InlineParagraphsWidget extends WidgetBase {
   +  /**
   +   *Available paragraphs types.
   +   *
   +   * @var array
   +   */
   +  protected $accessOptions = NULL;
   

   Available => Accessible?

5. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -748,33 +783,11 @@ class InlineParagraphsWidget extends WidgetBase {
   +    $field_state = static::getWidgetState($this->parents, $field_name, $form_state);
   

   the two set/get calls to this were there before, we're just renaming variables. I think changing that would be better be done in a separate issue.

6. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -815,100 +828,11 @@ class InlineParagraphsWidget extends WidgetBase {
   +    if (($this->realItemCount < $cardinality || $cardinality == FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED) && !$form_state->isProgrammed()) {
   +      $elements['add_more'] = $this->buildAddActions();
        }
   

   I think we should also open an issue to investigate this real item count stuff together with access.

   I find that highly confusing, as I said before, If there are 3 texts and a text + item (which I do not have access to) on a field that allows 4 values then the one I don't have access to will be removed and I will be allowed to add a nother one?

   IMHO, we need an access_denied state, that displays a message that you can't edit a paragraph. Similar to closed, but can't be opened.

   But that's a separate issue.

7. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -933,6 +857,164 @@ class InlineParagraphsWidget extends WidgetBase {
   +          '#type' => 'markup',
   +          '#markup' => '<em class"color-warning">' .
   +            $this->t('You are not allowed to add any of the @title types.', ['@title' => $this->getSetting('title')]),
   +        ];
   

   Not sure why we change this from #type container with classes to markup. Also we're not closing the em?

8. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -933,6 +857,164 @@ class InlineParagraphsWidget extends WidgetBase {
   +      return $add_more_elements ;
   +    }
   +
   +    if ($this->getSetting('add_mode') == 'button' || $this->getSetting('add_mode') == 'dropdown') {
   +      return $this->buildButtonsAddMode();
   +    }
   +
   +    return $this->buildSelectAddMode() ;
   

   spaces before ;

   Also not sure if I like not having an explicit else here. While it is not technically required I think it would help to increase readability. And we might add more option later on.

9. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -933,6 +857,164 @@ class InlineParagraphsWidget extends WidgetBase {
   +   * Returns the available paragraphs type.
   ...
   +  public function getAllowedTypesWithAccess() {
   +    if ($this->accessOptions !== NULL) {
   ...
   +    $bundles = $this->getAllowedTypes();
   

   The naming here is IMHO a bit strange, the method is getAllowedTypes() but the propertiy is accessOptions. If it is options then this should maybe be called getAccessibleOptions()

   Also documentation says *type* (singluar) and should clarify that it returns options the user has access to.

10. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
    @@ -933,6 +857,164 @@ class InlineParagraphsWidget extends WidgetBase {
    +      '#access' => !$this->isTranslating,
    

    we already check this outside, this access can go.

Implemented comment #27.
Created two new issues: #2834824: Two setWidgetState calls in formElementMultiple and #2834829: Add tests for per-type access checking.
And also if we delete '#access' => !$this->isTranslating, test are failing.

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -815,100 +828,11 @@ class InlineParagraphsWidget extends WidgetBase {
+    if (($this->realItemCount < $cardinality || $cardinality == FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED) && !$form_state->isProgrammed()) {
+      $elements['add_more'] = $this->buildAddActions();
     }

The #access is needed because I added an $this->isTranslating check directly to this, so we never even went inside. Looks like this was removed again, is there a reason for that?

Implemented comment #30.

That code confused me as well for a second, but we are looking for *more than one* there, not at least one.

I guess this is ready then.

Almost committed, but those methods should be protected...

Anyway... decided to do dirty quickfix and commit it with on-the-fly changingn to protected... Crozz fingerz for tests to pass! :-)