Argentina
Account created on 15 June 2007, over 17 years ago
  • Senior Developer at Gizra 
#

Merge Requests

Recent comments

🇦🇷Argentina dagmar Argentina

Alright. I finished my idea in the merge request.

The plan is to override the default auth provider if needed. So, no api changes, and safe defaults for sites that do not specify any override.

For developers the only new api change is the method

$restClient->overrideAuthProvider('provider_id');

Which changes the auth provider.

I haven't had the change to fix the test yet, but if someone want's to take it, feel free to do it.

The UI has been also modified to make clear that the auth provider set is overriding the default one.

🇦🇷Argentina dagmar Argentina

I've been playing with the latest patch for some time now, my general felling is the approach of making provider_id = '' an optional parameter is very dangerous and complicates a lot the code.

I think instead the provider_id should be a required parameter, and provide a hook_update_n to create a default one based on the current config. This will of course be a breaking compatibility change for custom code out there, but in the other hand tools like phpstan will allow to detect how to fix this properly.

The same should apply to auth manager.

I miss dreditor to review this .patch format, but I will do my best:

+++ b/modules/salesforce_mapping/src/Entity/MappedObject.php
@@ -409,7 +409,8 @@ class MappedObject extends RevisionableContentEntityBase implements MappedObject
       $result = $this->client()->objectUpdate(
         $mapping->getSalesforceObjectType(),
         $this->sfid(),
-        $params->getParams()
+        $params->getParams(),
+        $mapping->provider_id

Use $mapping->getProviderId() instead.

@@ -203,14 +203,17 @@ class RestClient implements RestClientInterface {
   /**
    * {@inheritdoc}
    */
-  public function isInit() {
-    if (!$this->authProvider || !$this->authManager) {
+  public function isInit($provider_id = '') {
+    if (!$this->authProvider || !$this->authManager->getProvider($provider_id)) {
       return FALSE;
     }
     // If authToken is not set, try refreshing it before failing init.
     if (!$this->authToken) {
       try {
         $this->authToken = $this->authManager->refreshToken();
+        if (!empty($provider_id)) {
+          $this->authToken = $this->authManager->refreshToken($provider_id);
+        }
         return isset($this->authToken);
       }
       catch (\Exception $e) {
@@ -224,20 +227,26 @@ class RestClient implements RestClientInterface {
   /**
    * {@inheritdoc}
    */
-  public function apiCall($path, $params = [], $method = 'GET', $returnObject = FALSE, array $headers = []) {
-    if (!$this->isInit()) {
+  public function apiCall($path, $params = [], $method = 'GET', $returnObject = FALSE, array $headers = [], $provider_id = '') {

I think the provider Id should be required when initializing the rest client, and the provider id should not be set after the client is initialized, this allows to remove provider_id from all the methods of this class.

🇦🇷Argentina dagmar Argentina

Thanks, looks good to me as well.

The method addFilterToQuery(Request $request, SelectInterface &$query) needs some documentation updates. The return type is not properly documented.

🇦🇷Argentina dagmar Argentina

51 errors to go

------ ----------------------------------------------------------------------------------------------------------- 
  Line   Entity/OgMembership.php                                                                                    
 ------ ----------------------------------------------------------------------------------------------------------- 
  322    Parameter #2 $callback of function array_filter expects (callable(Drupal\og\OgRoleInterface): bool)|null,  
         Closure(Drupal\og\Entity\OgRole): bool given.                                                              
  325    Parameter #1 $callback of function array_map expects (callable(Drupal\og\OgRoleInterface): mixed)|null,    
         Closure(Drupal\og\Entity\OgRole): (int|string|null) given.                                                 
  401    Parameter #2 $callback of function array_filter expects (callable(Drupal\og\OgRoleInterface): bool)|null,  
         Closure(Drupal\og\Entity\OgRole): bool given.                                                              
  616    Call to an undefined method Drupal\Core\Entity\EntityInterface::getGroup().                                
 ------ ----------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------ 
  Line   MembershipManager.php                                                                           
 ------ ------------------------------------------------------------------------------------------------ 
  55     Interface Drupal\Core\Entity\EntityTypeManagerInterface referenced with incorrect case:         
         Drupal\core\Entity\EntityTypeManagerInterface.                                                  
  308    Parameter #1 $group of method Drupal\og\OgMembershipInterface::setGroup() expects               
         Drupal\Core\Entity\ContentEntityInterface, Drupal\Core\Entity\EntityInterface given.            
  355    Call to an undefined method Drupal\Core\Entity\EntityInterface::get().                          
  364    Call to an undefined method Drupal\Core\Entity\EntityInterface::get().                          
  472    Instanceof between int and Drupal\Core\Session\AccountInterface will always evaluate to false.  
         💡 Because the type is coming from a PHPDoc, you can turn off this check by setting             
            treatPhpDocTypesAsCertain: false in your phpstan.neon.                                       
  486    Instanceof between int and Drupal\Core\Session\AccountInterface will always evaluate to false.  
         💡 Because the type is coming from a PHPDoc, you can turn off this check by setting             
            treatPhpDocTypesAsCertain: false in your phpstan.neon.                                       
  498    Instanceof between int and Drupal\Core\Session\AccountInterface will always evaluate to false.  
         💡 Because the type is coming from a PHPDoc, you can turn off this check by setting             
            treatPhpDocTypesAsCertain: false in your phpstan.neon.                                       
  553    Method Drupal\og\MembershipManager::loadGroups() should return                                  
         array<array<Drupal\Core\Entity\ContentEntityInterface>> but returns                             
         array<array<Drupal\Core\Entity\EntityInterface>>.                                               
 ------ ------------------------------------------------------------------------------------------------ 

 ------ ----------------------------------------------------------------------------------------------------- 
  Line   Og.php                                                                                               
 ------ ----------------------------------------------------------------------------------------------------- 
  126    Method Drupal\og\Og::createField() should return Drupal\Core\Field\FieldConfigInterface but returns  
         Drupal\field\FieldConfigInterface.                                                                   
  185    Call to an undefined method Drupal\og\MembershipManagerInterface::getGroupMemberships().             
  205    Parameter #2 $user of method Drupal\og\MembershipManagerInterface::createMembership() expects        
         Drupal\user\UserInterface, Drupal\Core\Session\AccountInterface given.                               
 ------ ----------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------- 
  Line   OgAccess.php                                                                                                 
 ------ ------------------------------------------------------------------------------------------------------------- 
  197    If condition is always true.                                                                                 
         💡 Because the type is coming from a PHPDoc, you can turn off this check by setting                          
            treatPhpDocTypesAsCertain: false in your phpstan.neon.                                                    
  249    Call to an undefined method Drupal\Core\Access\AccessResultInterface::addCacheTags().                        
  295    Call to method addCacheTags() on an unknown class Drupal\Core\Access\RefinableCacheableDependencyInterface.  
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols                                          
  295    PHPDoc tag @var for variable $result contains unknown class                                                  
         Drupal\Core\Access\RefinableCacheableDependencyInterface.                                                    
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols                                          
  319    Parameter #2 $group of class Drupal\og\Event\GroupContentEntityOperationAccessEvent constructor expects      
         Drupal\Core\Entity\ContentEntityInterface, Drupal\Core\Entity\EntityInterface given.                         
  319    Parameter #3 $groupContent of class Drupal\og\Event\GroupContentEntityOperationAccessEvent constructor       
         expects Drupal\Core\Entity\ContentEntityInterface, Drupal\Core\Entity\EntityInterface given.                 
 ------ ------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------- 
  Line   OgRoleManager.php                                                                                              
 ------ --------------------------------------------------------------------------------------------------------------- 
  125    Method Drupal\og\OgRoleManager::getRequiredDefaultRoles() should return array<Drupal\og\Entity\OgRole> but     
         returns array<string, Drupal\Core\Entity\EntityInterface>.                                                     
  136    Method Drupal\og\OgRoleManager::getRolesByBundle() should return array<Drupal\og\OgRoleInterface> but returns  
         array<Drupal\Core\Entity\EntityInterface>.                                                                     
  165    Method Drupal\og\OgRoleManager::getRolesByPermissions() should return array<Drupal\og\OgRoleInterface> but     
         returns array<Drupal\Core\Entity\EntityInterface>.                                                             
  188    Negated boolean expression is always false.                                                                    
         💡 Because the type is coming from a PHPDoc, you can turn off this check by setting                            
            treatPhpDocTypesAsCertain: false in your phpstan.neon.                                                      
 ------ --------------------------------------------------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------------------------------------- 
  Line   PermissionManager.php                                                                               
 ------ ---------------------------------------------------------------------------------------------------- 
  61     Method Drupal\og\PermissionManager::getDefaultGroupPermissions() should return                      
         array<Drupal\og\GroupPermission> but returns array<Drupal\og\PermissionInterface>.                  
  81     Method Drupal\og\PermissionManager::getDefaultEntityOperationPermissions() should return            
         array<Drupal\og\GroupContentOperationPermission> but returns array<Drupal\og\PermissionInterface>.  
 ------ ---------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------- 
  Line   Plugin/EntityReferenceSelection/OgSelection.php                                      
 ------ ------------------------------------------------------------------------------------- 
  121    Call to an undefined method                                                          
         Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface::buildEntityQuery().  
  150    If condition is always true.                                                         
  159    If condition is always true.                                                         
 ------ ------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------- 
  Line   Plugin/Field/FieldFormatter/GroupSubscribeFormatter.php                                                        
 ------ --------------------------------------------------------------------------------------------------------------- 
  158    If condition is always true.                                                                                   
         💡 Because the type is coming from a PHPDoc, you can turn off this check by setting                            
            treatPhpDocTypesAsCertain: false in your phpstan.neon.                                                      
  193    Left side of && is always true.                                                                                
         💡 Because the type is coming from a PHPDoc, you can turn off this check by setting                            
            treatPhpDocTypesAsCertain: false in your phpstan.neon.                                                      
  193    Variable $access in PHPDoc tag @var does not exist.                                                            
  198    Left side of && is always true.                                                                                
         💡 Remove remaining cases below this one and this error will disappear too.                                    
  218    Offset 'title' on array{title: Drupal\Core\StringTranslation\TranslatableMarkup, url: Drupal\Core\Url, class:  
         array{'unsubscribe'}} in empty() always exists and is not falsy.                                               
 ------ --------------------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------------- 
  Line   Plugin/Field/FieldWidget/OgComplex.php                                                                  
 ------ -------------------------------------------------------------------------------------------------------- 
  138    Call to an undefined method Drupal\Core\TypedData\TypedDataInterface::get().                            
  175    Access to an undefined property Drupal\Core\TypedData\TypedDataInterface::$_weight.                     
         💡 Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property                    
  285    Parameter #3 $weight_delta of method Drupal\og\Plugin\Field\FieldWidget\OgComplex::otherGroupsSingle()  
         expects int, float given.                                                                               
  305    Parameter #3 $weight_delta of method Drupal\og\Plugin\Field\FieldWidget\OgComplex::otherGroupsSingle()  
         expects int, float given.                                                                               
 ------ -------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------- 
  Line   Plugin/Validation/Constraint/UniqueOgMembershipConstraintValidator.php                                         
 ------ --------------------------------------------------------------------------------------------------------------- 
  56     Method Drupal\og\Plugin\Validation\Constraint\UniqueOgMembershipConstraintValidator::validate() has parameter  
         $value with no type specified.                                                                                 
  58     Variable $value in isset() always exists and is not nullable.                                                  
  89     Access to an undefined property Symfony\Component\Validator\Constraint::$notUniqueMembership.                  
         💡 Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property                           
 ------ --------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------- 
  Line   Plugin/Validation/Constraint/ValidOgRoleConstraintValidator.php                                                
 ------ --------------------------------------------------------------------------------------------------------------- 
  18     Method Drupal\og\Plugin\Validation\Constraint\ValidOgRoleConstraintValidator::validate() has parameter $value  
         with no type specified.                                                                                        
  20     Variable $value in isset() always exists and is not nullable.                                                  
  25     Negated boolean expression is always false.                                                                    
         💡 Because the type is coming from a PHPDoc, you can turn off this check by setting                            
            treatPhpDocTypesAsCertain: false in your phpstan.neon.                                                      
  31     Call to an undefined method Drupal\Core\Entity\FieldableEntityInterface::getGroup().                           
  32     Call to an undefined method Drupal\Core\Entity\FieldableEntityInterface::getGroup().                           
  34     Call to an undefined method Drupal\Core\Field\FieldItemInterface::referencedEntities().                        
  36     Access to an undefined property Symfony\Component\Validator\Constraint::$notValidRole.                         
         💡 Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property                           
 ------ --------------------------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------------------------- 
  Line   Plugin/views/argument_default/Group.php                                                                    
 ------ ----------------------------------------------------------------------------------------------------------- 
  77     Method Drupal\og\Plugin\views\argument_default\Group::buildOptionsForm() has parameter $form with no type  
         specified.                                                                                                 
  126    Method Drupal\og\Plugin\views\argument_default\Group::getGroup() should return                             
         Drupal\Core\Entity\ContentEntityInterface|null but return statement is missing.                            
 ------ ----------------------------------------------------------------------------------------------------------- 

 [ERROR] Found 51 errors                                                                                                

🇦🇷Argentina dagmar Argentina

dagmar created an issue.

🇦🇷Argentina dagmar Argentina

We replicated the problem in one of our sites. A QA analyst verified the patch of Merge request 33 fixes the problem. I reviewed the code and looks good, concerns in #17 are addressed by #19 and #20.

🇦🇷Argentina dagmar Argentina

@ckrina thanks good to know :) So the navigation module will allow a color change and the environment indicator will have to interact with this new API? No CSS changes are required from this module if I understand this correctly? Any issue in core to check this?

🇦🇷Argentina dagmar Argentina

Added Allow kernel tests to fail or expect logged errors Needs work to the issue summary to, to proper fix the problem with tests. Thanks @joachim

🇦🇷Argentina dagmar Argentina

dagmar changed the visibility of the branch 2401463-dblog-entities to hidden.

🇦🇷Argentina dagmar Argentina

dagmar changed the visibility of the branch 2401463-dblog-entities-D10 to hidden.

🇦🇷Argentina dagmar Argentina

Isn't it possible that it will get too complicated for normal use? I know that uninstalling this module is not something, that is done on a daily basis, but even so.. We can argue that it is needed for all modules using content enties, but I still consider dblog entries as something different like a real content (for me these are just logs).

@poker10 I found a way 📌 Make dblog entities Postponed to not need this to implement 📌 Make dblog entities Postponed

I will keep this issue open in case someone else want to work on this, but the original problem described in the issue is not a problem anymore.

🇦🇷Argentina dagmar Argentina

I found a way to not have to pause the creation of logs and remove the dblog module transparently as usual.

The approach is not elegant, but it works. When dblog module is asked if has data from the ContentUninstallValidator it replies no... And the module can be uninstalled. Logs are deleted anyways by the dblog_module_preuninstall hook. There is no other way to implement that I'm aware of as the ModuleInstaller::uninstall method does not offer any other opportunity to modify the validations.

This means this issue is not longer postponed on Provide a way to disable creation of database log entries Needs work

🇦🇷Argentina dagmar Argentina
$config['environment_indicator.indicator']['bg_color'] = '#00a073';
$config['environment_indicator.indicator']['fg_color'] = '#ffffff';

With this configuration and this patch I see this.

🇦🇷Argentina dagmar Argentina

Another interesting approach to analyze is the UI provided by the paragraph module. As it allows you to delete the content "on the fly" when deleting a paragraph type.

🇦🇷Argentina dagmar Argentina

@poker10 I think you pointed out some valid inputs. I had the same thoughts as well and this is why I tried to avoid this in the first place while coding 📌 Make dblog entities Postponed

Unfortunately I don't see an easy way to avoid this, at least without not touching a lot of content related stuff involved with content deletion.

The way I see it, dblog entries will be the same than Comments now. If you have a non public site, deleting the logs will be enough to not need to pause them.

I'm thinking maybe we can avoid this and improve the UX at the same time to allow you to block temporally log entries while you are deleting them.

I think we can also play with ?redirect=to in the url to make this as simple as possible.

Eventually most developers will use drush to uninstall dblog, so I think we can provide a patch to make this in one go if we need to.

🇦🇷Argentina dagmar Argentina

Great to see a new validation! However I think the proper validation should be a number 0 or greater. We don't know if someone is altering this value with custom code and the only thing we need to ensure to have dblog working properly is row_limit an unsigned integer.

🇦🇷Argentina dagmar Argentina

I think this will require a change record as it is changing the constructor signature.

🇦🇷Argentina dagmar Argentina

Thanks @mfb. I think the service is a good idea. But I won't be able to work on this for some time, so #47 for now is a good option as well.

🇦🇷Argentina dagmar Argentina

One more fix. Before, clicking form descriptions was re-sending any form.

🇦🇷Argentina dagmar Argentina

I've been trying this today. It seems the approach to alter the service is not actually necessary. Dblog can provide the service using by DblogServiceProvider::register and provide the regular logger only when the config is not available.

The only example in core I found doing something kind of similar is the Drupal\language\LanguageServiceProvider.

The logic then would be something like this:


namespace Drupal\dblog;

use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\DependencyInjection\ServiceProviderBase;

class DblogServiceProvider extends ServiceProviderBase {

  public function register(ContainerBuilder $container) {

    if (\Drupal::configFactory()->getEditable('dblog.settings')->get('logging_paused')) {
      return;
    }

    $container->register('dblog.logger', 'Drupal\dblog\Logger\DbLog')
    // ...
  }

}

\Drupal::configFactory() wont work at this register phase I'm afraid. But again LanguageServiceProvider provides some clues how what to do.

🇦🇷Argentina dagmar Argentina

In the formatMessage() method, the variables are unserialized and the backtrace markup object is created for use in the message. It would be redundant to repeat these two steps again in the eventDetails()

@mfb I see. Thanks!

Are we re-introducing #2371141: XSS vulnerability when displaying exception backtrace because we are not sanitizing the output? I would love to see a test case for this new addition.

🇦🇷Argentina dagmar Argentina

Ok, one more round, this patch is bigger than #10 because I went over all the occurrences of once.

🇦🇷Argentina dagmar Argentina

@mfb Any particular reason why

          // Save a reference so the backtrace can be displayed separately.
          if (!str_contains($row->message, '@backtrace_string')) {
            $row->backtrace = $variables['@backtrace_string'];
          }

Can be calculated inside the eventDetails method? The logic here seems a bit odd to follow. Also with change from #52 ::formatMessage would do more than just formatting the message.

🇦🇷Argentina dagmar Argentina

Once more once required in js/form-toggle-description.js

🇦🇷Argentina dagmar Argentina

Sorry for reopening this, but the commit made here https://www.drupal.org/project/rollbar/issues/3289422#comment-15183543 📌 Drupal 10 compatibility fixes Fixed is not the right fix.

As I learned more about D10 upgrades I think the right fix here is:

Have one release to be D9 compatible only, and have another one to be D10 compatible only, and then advice to use this composer version:

"drupal/rollbar: "3 || 4",

Assuming v4 will be only D10 compatible.

🇦🇷Argentina dagmar Argentina

I ran upgrade_status to check what are the missing manual fixes to apply. This patch seems to cover them all.

🇦🇷Argentina dagmar Argentina

@intrafusion I created issue #3379795 with the intention of not committing it. If you apply my fix the module won't be compatible with D10. This is why I mentioned it is something hard to fix, you can't be compatible with the 2 PSR3 signatures at the same time.

🇦🇷Argentina dagmar Argentina

I'm leaving the patch here. This is only required if you plan to upgrade rollbar to the latest version while you are still using Drupal 9 (but plan to upgrade to D10 soon).

🇦🇷Argentina dagmar Argentina

So the problem here is with the update but not with the fresh install? We ran all the tests for Sqlite, Mysql and Postgres https://www.drupal.org/project/drupal/issues/3081144#comment-15012792 🐛 Database primary keys can exceed maximum integer storeable (has actually occurred, in watchdog) Fixed is this a lack of tests?

🇦🇷Argentina dagmar Argentina

Looks good, and fixes a problem that otherwise it quite complicated to recover from (see the original issue description). However as is this patch will not apply to PHP 7.4 as Stringable is only PHP 8 available.

🇦🇷Argentina dagmar Argentina

What about adding the SVG content instead of the path to an icon as part of the json?

[
    {
      "title": "Link",
      "icon": '<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M8 16h8v2H8zm0-4h8v2H8zm6-10H6c-1.1 0-2 .9-2 2v16c0 1.1.89 2 1.99 2H18c1.1 0 2-.9 2-2V8l-6-6zm4 18H6V4h7v5h5v11z"/></svg>
',
      "description": "Insert a link to the Drupal CKEditor5 project site.",
      "html": "<p>Do you know this cool <a href='https://www.drupal.org/project/ckeditor5' target='_blank'>editor</a>?</p>"
    },
    {
        "title": "Simple Table",
        "icon": '<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M8 16h8v2H8zm0-4h8v2H8zm6-10H6c-1.1 0-2 .9-2 2v16c0 1.1.89 2 1.99 2H18c1.1 0 2-.9 2-2V8l-6-6zm4 18H6V4h7v5h5v11z"/></svg>
',
        "description": "Insert a simple Table.",
        "html": "<table border='1'><tr><th>Header 1</th><th>Header 2</th><th>Header 3</th></tr><tr><td>Row 1, Column 1</td><td>Row 1, Column 2</td><td>Row 1, Column 3</td></tr><tr><td>Row 2, Column 1</td><td>Row 2, Column 2</td><td>Row 2, Column 3</td></tr><tr><td>Row 3, Column 1</td><td>Row 3, Column 2</td><td>Row 3, Column 3</td></tr></table>"
    }
  ]

🇦🇷Argentina dagmar Argentina

A minimalist approach if you don't want to deal with user input json in the ui is the use the getDynamicPluginConfig to retrieve the path of the template, and use $this->moduleHandler->alter() to allow other modules to alter the path passing the $editor as argument. Not sure how this gets cached though.

🇦🇷Argentina dagmar Argentina

@dbielke1986 thanks for the quick response. I'm not fully familiar with CkEditor plugins yet, but I've been inspecting the code and checking different alternative approaches.

I think my proposal of services is not necessary, we can achieve the same result using CKEditor Plugins. The most similar plugin I found for this task is the core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/SourceEditing.php plugin. It provides a text area that could be use to define the json of the expected templates there. And exposes this config inside via getDynamicPluginConfig.

Inside the Ckeditor JS plugin you can access this settings using: this.editor.plugins.get('name_of_your_config_key');

You can find another full example in web/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/Media.php and how it loads the config in core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalmediaui.js

This approach will save a template per filter format. While it is not fully flexible at least allows to have multiple configs instead of just one.

And eventually a moduleHandler->alter() could be added to allow other modules to alter the template before injecting it into the js.

🇦🇷Argentina dagmar Argentina

Create a possibility to specify the configuration file which should be used.

I would like to suggest to define this from a service. Even if the default service loads the value from a config form.

The reason to do this is to allow more complex workflows where different templates can be loaded based on the current user or the current wysiwyg/filter format.

🇦🇷Argentina dagmar Argentina

Thanks!. Fixed in release 1.0.2.

🇦🇷Argentina dagmar Argentina

Thanks! @Nikolas Haliotis

🇦🇷Argentina dagmar Argentina

There are a few coding standards that needs to be fixed, but those are irrelevant now. The only thing that is not 100% to me right now is how to make the DblogServiceProvider::alter code run as soon the Dblog Form is submitted.

I'm not sure if \Drupal::service('kernel')->rebuildContainer(); is the way to go, or if there is something less aggressive. I was not able to find something in core that does this already. All the calls to rebuildContainer I found are part of tests.

🇦🇷Argentina dagmar Argentina

@sboden Thanks for the bug report. I think this is a duplicate of #2952091: Show translatable db log message on the main view

🇦🇷Argentina dagmar Argentina

I tried to replicate this with Drupal 9.5.x and 10.x. In both scenarios, the default value for 'identity' is set to drupal. And this has been the case since 2012 according to git log.

If you submit the form UI for syslog with no identity, it fallbacks to an empty string.

As @mfb said https://www.drupal.org/project/drupal/issues/3333215#comment-14868399 🐛 Return early if syslog configs are NULL to avoid openlog deprecation Fixed the only way this can happen is by either, editing the identity using drush cedit, or by manually removing the identity value from the config file and running drush config sync.

This seems more a won't fix to me than forcing the value of openlog.

🇦🇷Argentina dagmar Argentina

The patch looks good, but in my opinion the change record needs to be a bit more verbose on other ways the watchdog_exception needs to be replaced:

Only the 2 first parameters of this function are covered:

function watchdog_exception($type, Exception $exception, $message = NULL, $variables = [], $severity = RfcLogLevel::ERROR, $link = NULL)

There is no example of how to use $link and other errors than RfcLogLevel::ERROR.

🇦🇷Argentina dagmar Argentina

Tagging this with Novice as the instructions to proceed are well defined. I'm here to review and provide guidance if needed.

🇦🇷Argentina dagmar Argentina

I had more time to think about item 1. I think we should not make an exception for dblog entries. After researching a bit why the content validator was created it seems a bit dangerous to bypass the checks.

Concerns about disabling the dblog module in a production environment are still valid though. I created this to take care of this functionality: Provide a way to disable creation of database log entries Needs work

🇦🇷Argentina dagmar Argentina

I went over all the pending open threads of the merge request and closed most of them. I left a few open to make sure the provided answer is enough for the original reviewers.

I also checked some concerns I had related caching. It seems entities are not cached but routes are (example route:[language]=en:/admin/reports/dblog/event/10) in cache_data table. This is also happening for the current non entity implementation so I will not attempt to fix this as part of this issue. Maybe is not even an issue.

Most of the concerns regarding performance, indexing and future compatibility with 🐛 Database primary keys can exceed maximum integer storeable (has actually occurred, in watchdog) Fixed are addressed as the underlying system stills uses the same database scheme and insert queries.

My general feeling about the code (as the current dblog maintainer) is: this is pretty much ready to be in. There is some room of improvement in the DblogEntryViewBuilder, but probably qualifies for a follow up. And comparing this with the current code of the dblog module I would say this is a big improvement in code quality and alignment of Drupal standards. And will also allow to close several postponed issues for more than a few years now.

The remaining steps in my opinion are:

  1. Reviewing the latest changes to ContentUninstallValidator::validate
  2. as this is calling a new API method: ContentEntityTypeInterface;:requireDeleteContentBeforeUninstall which needs some grammar and common sense checking.

  3. Review if all annotations defined in DblogEntry are correct, or if something is missing or redundant.
  4. Get some last code review round paying specially attention to the high level picture of this. I already did this a few times, but I wrote most of the code, and doesn't feel right to assume it is right.
🇦🇷Argentina dagmar Argentina

I updated the issue summary based on the latest reviews assuming the proposed method names are ok. Removing needs profiling as I did the profiling tests in #61.

🇦🇷Argentina dagmar Argentina

I updated the issue summary to describe the proposed solution.

🇦🇷Argentina dagmar Argentina

Upgraded to work with Drupal 10.1.x. The lines from patch #49 regarding core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php now seems included in another file core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php.

I think I also addressed comment .3 from #48 🐛 Database primary keys can exceed maximum integer storeable (has actually occurred, in watchdog) Fixed

🇦🇷Argentina dagmar Argentina

@smustgrave This is what I have in mind. Something similar to what I mentioned in #8.

Using this approach, the signature of the Logger is not affected.

There is one test that is failing for me locally, I'm not sure how to fix it. Maybe @mfb can figure out why.

1) Drupal\Tests\syslog\Functional\SyslogTest::testSettings
Symfony\Component\DependencyInjection\Exception\RuntimeException: Unable to dump a service container if a parameter is an object without _serviceId.

/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:436
/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:316
/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:230
/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:134
/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:75
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:935
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:810
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:600
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:236
/var/www/html/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:465
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:545
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:364
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
🇦🇷Argentina dagmar Argentina

I updated the issue summary to answer @catch concerns regarding indexes. I ran a `DESCRIBE watchdog;` command before and after installing the site using this patch, and the table structure remains the same.

I guess this also is valid for the issue 📌 Make dblog entities Postponed , if we fix this in the dblog.install module, the changes will also apply to dblog as entities.

I also addressed the comments about dblog exception in ContentUninstallValidator. Based on my current understanding of the module_installer logic, there is no way for dblog to by pass this content validator exception, so I had to modify it any way. I tried to make it as simple and clear possible, and not only for dblog.

🇦🇷Argentina dagmar Argentina

Rerolled for Drupal 9.5.x.

core/tests/Drupal/KernelTests/Core/Database/DriverSpecificSchemaTestBase.php has a lot of new lines in D10 than D9.5. I hope this doesn't break anything...

🇦🇷Argentina dagmar Argentina

I'm back to work on this.

🇦🇷Argentina dagmar Argentina

I faced this issue today. I seems to be a problem when installing and removing submodules which provide libraries. This issue is not automatically fixed by clearing the cache. I'm also not using Layout builder, so the workaround described in #2 🐛 Can't add node that has chart field on it, get WSOD Closed: outdated does not work for me.

In my case I installed the charts module. Then I created a chart, then I installed charts_google, and when I visited the old chart I got the described fatal error.

🇦🇷Argentina dagmar Argentina

I would like to review this as well. A bit concerned about the backward compatibility.

🇦🇷Argentina dagmar Argentina

@smustgraveI think what @mfb is trying to say is, send the 4xx code as part of the $context array. Because it is hidden behind the generic client error.

🇦🇷Argentina dagmar Argentina

Thanks I had to rollback some untended changed but the issue is fixed now. Will be part of beta4.

🇦🇷Argentina dagmar Argentina

I took another look to the patch and it is looking really good.

However after reading again the issue summary and thinking this feature being used by all sort of drupal (big and small) sites I wonder if we should provide a way to decide wether Severity Level filter should be more customizable. (Greater than certain level >= or just equal to the defined setting ==)

The oldest request I found related to this issue is: #77875: Allow administrator to set logging level for Watchdog . The idea of Severity greater or lower than certain values was proposed there too. I was the one that proposed in #72 that this should be changed to == but now thinking this more carefully I'm not sure if we should support both options. Or if we opt in for one of them, at least think better what is the best option here.

Having == gives enough flexibility to select what to ignore and what not. But using >= allow users to reduce the amount of settings to write in case you want to ignore a group of different severity levels for some particular channel or logger.

I understand this adds complexity to the patch and also requires some extra considerations of how to define if the severity check should be greater, equal or lower than the specified. Also brings some collision situations (where you define < Error and > Warning in two different options.

So please, share your thoughts on this.

Regarding the current code:

+++ b/core/tests/Drupal/Tests/Core/Logger/LoggerChannelTest.php
@@ -53,7 +63,31 @@ public function testLog(callable $expected, Request $request = NULL, AccountInte
+   * @throws \ReflectionException

This is no longer the case. Since you are changing the visibility scope in LoggerChannelTestable.

Another thing I notice is that there is no use of the Splat operator in core yet. There is a relevant issue here: #2551661: replace call_user_func_array() with splat (variadics) which is quite old and the reasons that blocked the process are no longer valid today.

🇦🇷Argentina dagmar Argentina
+++ b/core/modules/system/tests/src/Kernel/Logging/LoggingTest.php
@@ -0,0 +1,316 @@
+  public static $modules = ['syslog', 'syslog_test'];
+  protected $logFileName;

syslog and syslog_test module are no longer required as far I can see.

There are a few coding standards warnings reported too:

/var/lib/drupalci/workspace/jenkins-drupal_patches-68713/source/core/modules/system/tests/src/Kernel/Logging/LoggingTest.php
line 190	A comma should follow the last multiline array item. Found: 16
210	A comma should follow the last multiline array item. Found: 16
230	A comma should follow the last multiline array item. Found: 16
250	A comma should follow the last multiline array item. Found: 16
270	A comma should follow the last multiline array item. Found: 15
290	A comma should follow the last multiline array item. Found: 14
310	A comma should follow the last multiline array item. Found: 8

Also, it could be nice to add a Kernel test to dblog module to test that no logs are created when the settings are set to ignore page not found, like the example of default.settings.php

🇦🇷Argentina dagmar Argentina

I did a final review of the patch. And since I have no extra comments to add, the code looks good, with good test coverage, and all my concerns has been addressed, I'm marking this as RTBC.

@Loparev great work! You should probably expect an extra requests of changes by other eyes that may see things that I don't see.

Just a minor, minor thing (that i'm not totally sure anyway).

  1. +++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
    @@ -79,7 +80,14 @@ class LoggerChannel implements LoggerChannelInterface {
    +   * @var string[]|NULL
    

    @var array[string]bool|NULL

🇦🇷Argentina dagmar Argentina

This is looking really good to me. I did a full review again and just found a few changes that can improve the patch.

+++ b/core/modules/system/tests/src/Kernel/Logging/LoggingTest.php
@@ -0,0 +1,305 @@
+    $this->setSettings([]);

This line should be removed to be consistent with the settings defined before this patch.

+++ b/sites/default/default.settings.php
@@ -730,6 +730,68 @@
+ * will be ignored. Wildcard symbol can be used in with any parameter in
+ * any sequence.

The wildcard symbol can be used with any parameter in any sequence.

🇦🇷Argentina dagmar Argentina

I was doing a performance review of the code and noticed this. When you hit a not found page the, this is the how LoggerChannels methods are executed:

  1. __construct
  2. setRequestStack
  3. setCurrentUser
  4. setLoggers
  5. log
  6. sortLoggers
  7. isIgnoredLog
  8. __construct
  9. setRequestStack
  10. setCurrentUser
  11. setLoggers
  12. __construct
  13. setRequestStack
  14. setCurrentUser
  15. setLoggers

In another test, for example adding a notExistentFunction() to the user login form I get this call stack.

  1. __construct
  2. setRequestStack
  3. setCurrentUser
  4. setLoggers
  5. __construct
  6. setRequestStack
  7. setCurrentUser
  8. setLoggers
  9. log
  10. sortLoggers
  11. isIgnoredLog

(You can try this by yourself adding a var_dump at the beginning of each method of the class.)

Since the constructor is called 2 or 3 times in each execution, it could be interesting to move the code that reads the settings from the constructor outside. For example directly into isIgnoredLog. That the first time checks if the settings are initialized, and if they are not, load them from there.

🇦🇷Argentina dagmar Argentina
  1. +++ b/sites/default/default.settings.php
    @@ -729,6 +729,68 @@
    + * List of available severity levels:
    + *  - 'debug'
    + *  - 'info'
    + *  - 'notice'
    + *  - 'warning'
    + *  - 'error'
    + *  - 'critical'
    + *  - 'alert'
    + *  - 'emergency'
    

    This is still not addressed. I think we should use the LogLevel::EMERGENCY, LogLevel::ERROR, etc instead.

  2. +++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
    @@ -79,13 +80,51 @@ class LoggerChannel implements LoggerChannelInterface {
    +    return
    +      isset($this->ignoreLogs["{$channel}:{$level}:{$logger}"]) ||
    +      isset($this->ignoreLogs["{$channel}:{$level}:*"]) ||
    

    I think we could add a check to verify there something defined in $this->ignoreLogs.

    Like !empty($this->ignoreLogs)

    Because right now it will fail (but check) all the conditions if the array is empty.

🇦🇷Argentina dagmar Argentina

There are some error related to coding standards:

line 41	Expected 1 blank line before function; 2 found
44	TRUE, FALSE and NULL must be uppercase; expected "TRUE" but found "true"
101	Line indented incorrectly; expected 4 spaces, found 5
245	A comma should follow the last multiline array item. Found: ]
290	A comma should follow the last multiline array item. Found: ]
335	A comma should follow the last multiline array item. Found: ]
380	A comma should follow the last multiline array item. Found: ]
425	A comma should follow the last multiline array item. Found: ]
470	A comma should follow the last multiline array item. Found: ]
515	A comma should follow the last multiline array item. Found: ]
  1. +++ b/sites/default/default.settings.php
    @@ -730,6 +730,69 @@
    + * Drupal logging system can be configured in a way to ignore
    

    This can be organized to fit in a 80 line.

  2. +++ b/sites/default/default.settings.php
    @@ -730,6 +730,69 @@
    + *     'level' => \Psr\Log\LogLevel::DEBUG,
    ...
    + * List of available severity levels:
    

    This is not consistent with the use of level above.
    Maybe \Psr\Log\LogLevel::DEBUG, instead?

🇦🇷Argentina dagmar Argentina
  1. +++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
    @@ -86,6 +94,44 @@ class LoggerChannel implements LoggerChannelInterface {
    +      $level = $ignore_log["level"] == "*" ? $ignore_log["level"] : $this->levelTranslation[$ignore_log["level"]];
    

    Use single quotes instead of double.

  2. +++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
    @@ -86,6 +94,44 @@ class LoggerChannel implements LoggerChannelInterface {
    +    $full_match = in_array("{$channel}:{$level}:{$logger}", $this->ignoreLogs);
    

    I'm not really sure if this is the fastest way to check this. Makes sense from a readability point of view, but I wonder if there is another way to organize things to make this less cpu consuming.

    I'm not saying this is slow. But clearly it adds some extra checks before each log and with hundred of logs written by minute it could affect performance.

I'm not a algorithms expert. Maybe another person can take a look and give another point of view.

🇦🇷Argentina dagmar Argentina

You could create a new structure based on the settings defined in the settings.php. So you analyze the settings once, and create an optimized version to determinate if a log should be ignored or not.

🇦🇷Argentina dagmar Argentina

Yes @Loparev. I don't see a use case for * -> * -> * you could just disable the logging module in that case. Maybe we should analyze if the nested option is the best alternative here.

I'm thinking that maybe a plain set of configurations is more flexible and allow the use of `*` in a natural way. For example


$settings['ignore_logs'] = [
  [
    'channel' => 'page not found',
    'level' => '*'
    'logger' => 'Drupal\dblog\Logger\DbLog'
  ],
  [ 
    'channel' => 'php'
    'level' => 'info'
    'logger' => '*'
  ]
];
🇦🇷Argentina dagmar Argentina
  1. +++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
    @@ -86,6 +94,15 @@ class LoggerChannel implements LoggerChannelInterface {
    +    foreach (Settings::get('logs_suppression', []) as $channel => $severity_levels) {
    

    I'm not sure if this makes sense from a performance point of view. You are forcing to iterate through all the settings in the log function.

    If you just save the settings as they are defined in settings.php You can skip a few cycles by just checking if there are some keys defined.

  2. +++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
    @@ -133,7 +150,25 @@ public function log($level, $message, array $context = []) {
    +          $level > $this->levelTranslation[$setting_level] &&
    

    I'm not really sure that we want to ignore all the logs higher than this. I rather prefer to specify each log by separate. Or maybe introduce an 'all' options to exclude them all.

  3. +++ b/core/modules/system/tests/src/Kernel/Logging/LoggingTest.php
    @@ -0,0 +1,813 @@
    +  public static $modules = ['system', 'syslog', 'syslog_test', 'logging_test'];
    
    +++ b/sites/default/default.settings.php
    @@ -747,6 +747,62 @@
    + * Logging configuration.
    

    system is enabled by default.

  4. +++ b/core/modules/system/tests/src/Kernel/Logging/LoggingTest.php
    @@ -0,0 +1,813 @@
    +   * @param $count
    

    Missing descriptions here.

  5. +++ b/core/modules/system/tests/src/Kernel/Logging/LoggingTest.php
    @@ -0,0 +1,813 @@
    +    $this->assertLogCount(16, LoggingTest::CHANNEL_NAME_1, $log_records);
    

    You could replace CHANNEL_NAME_1 and CHANNEL_NAME_2 by CHANNEL_A and CHANNEL_B.
    To make them shorter.

  6. +++ b/sites/default/default.settings.php
    @@ -747,6 +747,62 @@
    + * Drupal core logging system can be configured in a way to suppress
    

    This is not just for core.

  7. +++ b/sites/default/default.settings.php
    @@ -747,6 +747,62 @@
    + * then this log record will be suppressed and will not be logged into the
    + * database as rule allows only records with severity level equals to or
    + * higher than 'info'. The rule is applicable only to that log records which
    + * are being logged to 'my_channel' channel from the \Drupal\dblog\Logger\DbLog
    + * logger.
    

    I've been thinking about the use of suppress, removed, filter here. I think the word we are looking for is 'ignore'. Since we are not deleting logs, just not persist them.

  8. +++ b/sites/default/default.settings.php
    @@ -747,6 +747,62 @@
    + *   'my_channel' => [
    

    Maybe here we could use the 'page not found' since this one of the the most used cases.

  9. +++ b/sites/default/default.settings.php
    @@ -747,6 +747,62 @@
    + * second rule will not be applied as there is another one defined earlier
    + * for the same channel and logger.
    

    I think this could be deleted if the > operator is changed to == in the LoggerChannel class.

  10. +++ b/sites/default/default.settings.php
    @@ -747,6 +747,62 @@
    + * severity level name. Core channels are defined in a core.services.yml
    

    I'm afraid this is incomplete. Most of the channels are dynamic and they are defined when you call \Drupal::logger().

    I list of the ones called by core are:

    • 'access denied'
    • 'config_sync'
    • 'cron'
    • 'entity_reference'
    • 'example'
    • 'field'
    • 'file system'
    • 'file'
    • 'filter'
    • 'image'
    • 'locale'
    • 'migrate_drupal_ui'
    • 'migrate'
    • 'php'
    • 'responsive_image'
    • 'security'
    • 'system'
    • 'theme'
    • 'tracker'
    • 'user'
    • 'views'

    We may want to show small list of the most relevant.

🇦🇷Argentina dagmar Argentina

Thanks!

+++ b/sites/default/default.settings.php
@@ -747,6 +747,26 @@
+ * Drupal core logging system can be configured in a way to filter
+ * out log messages by severity level on logger and channel level.
+ *
+ * For example:
+ * @code
+ * $settings['log_filters'] = [
+ *   'content' => [
+ *     'info' => [
+ *       'Drupal\dblog\Logger\DbLog',
+ *     ],
+ *   ]
+ * ];
+ * @endcode

Reading this documentation is not 100% clear to me how this could work. I think this needs to include some extra examples of:

  1. How users can know which available options are ('content', 'php', etc)?
  2. What 'info' means, are other options available?
  3. Why there is a Logger after all, does this means an excluded log will be logged for all the missing options?

Maybe a useful set of @see links to other documentation pages can enrich the explanations provided here.

Also. Is log_filters the right name? If we compare them with views filters for example or array_filter, they are used in the opposite way in the sense what you filter is what you get from results. Maybe logs_exclude or logs_suppress is a better name for this setting.

Finally. There are 50+ coding standard messages that are reported here: https://www.drupal.org/pift-ci-job/1009521

Production build 0.71.5 2024