Fixed in 1.0.3
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
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.
@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.
Thanks @marvil07
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.
I think this will require a change record as it is changing the constructor signature.
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.
Random test failure.
One more fix. Before, clicking form descriptions was re-sending any form.
Random test failure.
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.
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.
Ok, one more round, this patch is bigger than #10 because I went over all the occurrences of once.
@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.
Once more once required in js/form-toggle-description.js
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.
[#3197514]
I ran upgrade_status to check what are the missing manual fixes to apply. This patch seems to cover them all.
@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.
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).
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?
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.
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>"
}
]
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.
@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.
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.
Thanks!. Fixed in release 1.0.2.
Thanks! @Nikolas Haliotis
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.
Awesome @petiar! Many thanks.
@sboden Thanks for the bug report. I think this is a duplicate of #2952091: Show translatable db log message on the main view →
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.
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.
Tagging this with Novice → as the instructions to proceed are well defined. I'm here to review and provide guidance if needed.
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
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:
- Reviewing the latest changes to ContentUninstallValidator::validate
- Review if all annotations defined in DblogEntry are correct, or if something is missing or redundant.
- 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.
as this is calling a new API method: ContentEntityTypeInterface;:requireDeleteContentBeforeUninstall which needs some grammar and common sense checking.
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.
I updated the issue summary to describe the proposed solution.
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
@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
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.
🐛 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.
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...
I'm back to work on this.
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.
Fixed in version 1.0.1
dagmar → created an issue.
I would like to review this as well. A bit concerned about the backward compatibility.
Fixed in release 1.0.0
@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.
Thanks
Thanks I had to rollback some untended changed but the issue is fixed now. Will be part of beta4.