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

Merge Requests

Recent comments

πŸ‡¦πŸ‡·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

Patch #3 does not work for me:

πŸ‡¦πŸ‡·Argentina dagmar Argentina

I think solution C proposed in #61 πŸ› Dependency on config storage causes circular reference in service container Needs review is totally aligned with what I suggested in #4 πŸ› Dependency on config storage causes circular reference in service container Needs review . Thanks! @mfb

πŸ‡¦πŸ‡·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

Thanks @marvil07

πŸ‡¦πŸ‡·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

dagmar β†’ created an issue.

πŸ‡¦πŸ‡·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

dagmar β†’ created an issue.

πŸ‡¦πŸ‡·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

Awesome @petiar! Many thanks.

πŸ‡¦πŸ‡·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
πŸ‡¦πŸ‡·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
πŸ‡¦πŸ‡·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

πŸ› Schema::changeField() has bug when changing regular serial field to big serial field Fixed was merged for D10 and I just provided the backport for D9.5 I think this is now unblocked.

πŸ‡¦πŸ‡·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

Fixed in release 1.0.0

πŸ‡¦πŸ‡·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.

Production build 0.69.0 2024