Problem/Motivation

A few messages are set without checking if the debug mode is active.

CommentFileSizeAuthor
#7 3238703-add_debug_mode_check_on_all_messages_set_by_messenger_service-7.patch1.52 KBpjbaert
#3 3238703-add_debug_mode_check_on_all_messages_set_by_messenger_service-3.patch2.34 KBpjbaert
#2 3238703-add_debug_mode_check_on_all_messages_set_by_messenger_service-2.patch2.27 KBpjbaert

Comments

pjbaert created an issue. See original summary.

pjbaert’s picture

pjbaert
Primary language Dutch
commented
Assigned: pjbaert » Unassigned
Status: Active » Needs review
StatusFileSize
new 3238703-add_debug_mode_check_on_all_messages_set_by_messenger_service-2.patch2.27 KB

I added the debug checks on the preSave and deleteCloudflareVideo function.
Please review.

pjbaert’s picture

pjbaert
Primary language Dutch
commented
StatusFileSize
new 3238703-add_debug_mode_check_on_all_messages_set_by_messenger_service-3.patch2.34 KB

Added a extra comment that explains the debug check.

tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented
Status: Needs review » Needs work 
  1. +++ b/src/Plugin/Field/FieldType/CloudflareVideoItem.php
@@ -233,6 +233,7 @@ class CloudflareVideoItem extends FileItem {
+    $configFactory = \Drupal::service('config.factory')->get('cloudflare_stream.settings');

@@ -311,6 +315,7 @@ class CloudflareVideoItem extends FileItem {
+    $configFactory = \Drupal::service('config.factory')->get('cloudflare_stream.settings');

    This is not needed as the config is already loaded on line +-100 $this->configFactory = $config_factory->get('cloudflare_stream.settings'); 

  2. +++ b/src/Plugin/Field/FieldType/CloudflareVideoItem.php
@@ -285,7 +286,10 @@ class CloudflareVideoItem extends FileItem {
+          if ($debugMessages = $configFactory->get('debug_messages')) {

@@ -327,7 +332,10 @@ class CloudflareVideoItem extends FileItem {
+      if ($debugMessages = $configFactory->get('debug_messages')) {

    Should we maybe refactor this in a service so we could for example do $this->CloudflareStream->debugEnabled()?

pjbaert’s picture

pjbaert
Primary language Dutch
commented
Assigned: Unassigned » pjbaert

Thanks for your review. I agree with the feedback & the service suggestion. I'll try to rework this.

pjbaert’s picture

pjbaert
Primary language Dutch
commented
Issue summary: View changes

updated issue summary

pjbaert’s picture

pjbaert
Primary language Dutch
commented
Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#3239401: Create a Cloudflare Stream Service
StatusFileSize
new 3238703-add_debug_mode_check_on_all_messages_set_by_messenger_service-7.patch1.52 KB 
+++ b/src/Plugin/Field/FieldType/CloudflareVideoItem.php
@@ -233,6 +233,7 @@ class CloudflareVideoItem extends FileItem {
+    $configFactory = \Drupal::service('config.factory')->get('cloudflare_stream.settings');

@@ -311,6 +315,7 @@ class CloudflareVideoItem extends FileItem {
+    $configFactory = \Drupal::service('config.factory')->get('cloudflare_stream.settings');

>This is not needed as the config is already loaded on line +-100 $this->configFactory = $config_factory->get('cloudflare_stream.settings');

This was fixed in this patch.

>Should we maybe refactor this in a service so we could for example do $this->CloudflareStream->debugEnabled()?

Great suggestion, let's fix this in #3239401: Create a Cloudflare Stream Service

tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the cleanup and creation of the new issue to make a service!

  • pjbaert committed f3d468d on 8.x-2.x 
    Issue #3238703 by pjbaert, tim-diels: Add debug mode check on all...
pjbaert’s picture

pjbaert
Primary language Dutch
commented
Status: Reviewed & tested by the community » Fixed

Thanks for the review. This was pushed to the 2.x branch!

pjbaert’s picture

pjbaert
Primary language Dutch
commented
Assigned: pjbaert » Unassigned

Status: Fixed » Closed (fixed)

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