• Refactor the *ProcessMultipleForm classes into the one classe:
    • CloudProcessMultipleForm
    • CloudConfigProcessMultipleForm
    • CloudServerTemplateDeleteMultipleForm
CommentFileSizeAuthor
#5 3105833-5.patch34.97 KByas
#3 3105833-3.patch40.77 KByas
#2 3105833-2.patch40.77 KByas

Comments

yas created an issue. See original summary.

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Status: Needs work » Needs review
StatusFileSize
new 3105833-2.patch40.77 KB

@all

Could you please review the patch?

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Issue summary: View changes
StatusFileSize
new 3105833-3.patch40.77 KB

Updated the patch to follow up the merget at 3105004-20.patch

baldwinlouie’s picture

baldwinlouie commented
Status: Needs review » Needs work

@yas, I've review the patch. I have the comments below.

In general, I don't think the CloudconfigLocationController.php and CloudConfigLocationBlock.php code should be removed in this patch. Perhaps reroll the patch after updating from git?

  1. +++ b/src/Controller/CloudConfigLocationController.php
@@ -136,16 +136,6 @@ class CloudConfigLocationController extends ControllerBase {
-    // Support loading all cloud configs based on a particular cloud_config
-    // type such as only aws_ec2 or k8s.
-    $cloud_service_provider = $this->request->get('cloud_service_provider') ?? NULL;
-    if (isset($cloud_service_provider)) {
-      $cloud_configs = $this->entityTypeManager()->getStorage('cloud_config')
-        ->loadByProperties([
-          'type' => $cloud_service_provider,
-        ]);
-    }
-

    I don't think these should be removed. 

  2. +++ b/src/Entity/CloudContentEntityBase.php
@@ -132,16 +132,20 @@ class CloudContentEntityBase extends ContentEntityBase implements EntityOwnerInt
-    $this->updateCache();
+    self::updateCache();

    Curious why we went to a static function for updateCache() 

  3. +++ b/src/Plugin/Block/CloudConfigLocationBlock.php
@@ -130,61 +117,10 @@ class CloudConfigLocationBlock extends BlockBase implements ContainerFactoryPlug
-  /**
-   * {@inheritdoc}
-   */
-  public function blockForm($form, FormStateInterface $form_state) {
-    $form = parent::blockForm($form, $form_state);
-    $config = $this->getConfiguration();
-    $form['cloud_service_provider'] = [
-      '#type' => 'select',
-      '#title' => $this->t('Cloud Service Provider'),
-      '#description' => $this->t('Select cloud service provider'),
-      '#options' => $this->getCloudServiceProviders(),
-      '#default_value' => $config['cloud_service_provider'] ?? '',
-      '#description' => $this->t('Choose a cloud service provider to display on
-      the map. Leave blank to display all'),
-    ];
-
-    return $form;
-  }
-
-  /**
-   * {@inheritdoc}
-   */
-  public function blockSubmit($form, FormStateInterface $form_state) {
-    if (!empty($form_state->getValue('cloud_service_provider'))) {
-      $this->configuration['cloud_service_provider'] = $form_state->getValue('cloud_service_provider');
-    }
-    else {
-      unset($this->configuration['cloud_service_provider']);
-    }
-  }
-
-  /**
-   * Get the enabled cloud service providers.
-   *
-   * @return array
-   *   An array of providers.
-   */
-  private function getCloudServiceProviders() {
-    $providers = [
-      '' => 'Show all',
-    ];
-    $bundles = $this->entityTypeBundleInfo->getBundleInfo('cloud_config');
-    if (count($bundles)) {
-      foreach ($bundles ?: [] as $key => $bundle) {
-        $providers[$key] = $bundle['label'];
-      }
-    }
-    return $providers;
-  }
-

    I'm not sure if this is meant to be removed from this patch? 

  4. +++ b/src/Plugin/Block/CloudConfigLocationBlock.php
@@ -235,11 +171,7 @@ class CloudConfigLocationBlock extends BlockBase implements ContainerFactoryPlug
-      $params = [];
-      if (isset($this->configuration['cloud_service_provider'])) {
-        $params['cloud_service_provider'] = $this->configuration['cloud_service_provider'];
-      }

    I don't think this should be removed?

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Status: Needs work » Needs review
StatusFileSize
new 3105833-5.patch34.97 KB

@baldwinlouie

Thank you for your review. Sorry that I made a mistake that I didn't make the source code up to date, therefore 3105833-3.patch didn't include 3105240.patch.

For 2. above, I needed to make updateCache() static since included some other clear-cache methods as follows. Therefore $this->updateCache(); needs to be the static function like self::updateCache(); .

diff --git a/src/Entity/CloudContentEntityBase.php b/src/Entity/CloudContentEntityBase.php
@@ -132,16 +132,20 @@ class CloudContentEntityBase extends ContentEntityBase implements EntityOwnerInt

-  private function updateCache() {
+  private static function updateCache() {
     // Clear caches to refresh action links.
-    \Drupal::cache('render')->deleteAll();
+    // Clear block and menu cache.
+    \Drupal::cache('menu')->invalidateAll();
+    \Drupal::service('cache.render')->deleteAll();
+    \Drupal::service('router.builder')->rebuild();
+    \Drupal::service('plugin.cache_clearer')->clearCachedDefinitions();
   }

I re-created the patch; so could you please review the patch again?

baldwinlouie’s picture

baldwinlouie commented
Status: Needs review » Reviewed & tested by the community

@yas, thank you for the update. it looks good to me.

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented

@baldwinlouie

Thank you for your review. I'll merge the patch to 8.x-1.x and 8.x-2.x and close this issue as Fixed.

  • yas committed 8da02e2 on 8.x-1.x 
    Issue #3105833 by yas, baldwinlouie: Refactor the *ProcessMultipleForm...

  • yas committed 50790c2 on 8.x-2.x 
    Issue #3105833 by yas, baldwinlouie: Refactor the *ProcessMultipleForm...
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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