🇫🇷France @o'briat

Could someone review the last patch ?

I open a feature request for the priority point: https://www.drupal.org/project/clamav/issues/3540722 ✨ ClamAv must have the highest priority on FileValidation Event Active

o'briat → created an issue.

@kim.pepper
To me #9 was with #12.

Since the tests are now OK, I put back the issue as "Needs review".

IMHO ClamAv should assure its FileImageDimensionsConstraintValidator get the higher priority since it should check original file for virus before any other manipulations (check, resizes, ...).
This is specially true for virus that target image resizing lib such as gd or imagick

I think the core problem is that FileImageDimensionsConstraintValidator changes the file size during a validation process, according to https://www.drupal.org/project/drupal/issues/3363745 📌 Deprecate file_validate_image_resolution() and replace with a service Postponed this validation process should only does checks and returns info/errors.

This quick fix is to update the object's file size after resizing it (done again in https://www.drupal.org/project/drupal/issues/3522463 🐛 FileValidationEvent may not report the correct file size Active ), the long way is (IMHO) to redefine the API/events workflow of file Validator.

Should this issue closed and marked as duplicated of https://www.drupal.org/project/drupal/issues/3522463 🐛 FileValidationEvent may not report the correct file size Active ?

Update description and put back the issue to needs review.
The "PHPUnit Build" pipeline task failed, but it doesn't seem to be related to this issue.

phpsab with docker

There's another usage of $image->save() in "navigation" experimental module core/modules/navigation/src/Form/SettingsForm.php:286.

But the resize is done after the validation.

I push the same patch to the new constraint part:
Works for me.
kerneltest is ok.

I think that until this core issue is fixed, $file->getSize(); should not be trusted and replaced by filesize($file->getFileUri()) on https://git.drupalcode.org/project/clamav/-/blob/2.1.x/src/Scanner/Daemo...

I'll try to provide a patch

Don't know if it's revelant, but file_validate_image_resolution was removed without replacement, see https://www.drupal.org/node/3363745 → & https://www.drupal.org/project/drupal/issues/3221793 📌 Move file upload validation from file.module to constraint validators Fixed

o'briat → made their first commit to this issue’s fork.

Add the relation to the new core issue https://www.drupal.org/project/drupal/issues/3522463 🐛 FileValidationEvent may not report the correct file size Active with the switch from hook_file_validate to FileValidationEvent .

Add a note about using phpsab with Docker

Remove db/nfs mention. Rephrase settings tips

To be clear this MR does :

• Add an "info block" that display the basic request info as viewed by drupal/symfony : scheme,host,port and client ip
• Add "requirements lines":
• httpS status
• Detected scheme, host & port
• Detected remote client IP
• Reverse proxy info with $settings['reverse_proxy'] status and some details to help its setup (headers)
• Add a warning if https is not detected
• Add a warning if any "forwarded" header is present but $settings['reverse_proxy'] is not enabled

Remaining tasks:

• Add tests
• Rephrase/reword messages to make them clearer

@osab : add a note that point to " Develop > Local server setup > Windows development environment > Installing Drupal with DDEV in WSL2 on Windows → " ?

Could we reorder IDE list to reflect the last Drupal Survey:  PHPStorm, VScode, Vim, Sublime Text & Atom ? 
Maybe remove other ones: Emacs, Eclipse, Komodo, NetBeans, jEdit &  GNU Global setup for Drupal ?

Here's a first shot, still missing:

• wording, spell cheking, grammar
• unit test ?
• Analysis of the forwarded header's "for" attributes
• Adding a card on status page with current detected scheme, host, port and remote address

Disable default vscode greedy suggestion

Fix settings.php : remove greedy suggestions and fix paths

Explain module's machine source

ericyellin →   It's maybe due to the fact that AWS changes IP constantly: https://symfony.com/doc/current/deployment/proxies.html#but-what-if-the-...

In this case it could be safer to set protocol as you did instead of trusting all IPs.

But beware that your solution does not fix the true client IP retrieval.

Check migrate_plus config file  prefix

o'briat → created an issue.

OK.
I still think that this structure will add a lot of overheads on projects with several environments.

Just for the record, I stumble into a design problem on Drupal Symfony Mailer config.
This module forces you to save full "send mail commands" into the config, I don't know if you following the same path, but I think it's rise several issues: security, config override, ...

https://www.drupal.org/project/symfony_mailer/issues/3524098 ✨ Settings custom send mail command on multiple env Active

I changed title and description, feel free to edit it.

I'll have a look at the "Experimental Symfony Mailer Module" initiative, it looks like they'll cross the same issue:

$config['system.mail']['mailer_dsn'] = [
  'scheme' => 'sendmail',
  'host' => 'default',
];
$config['system.mail']['mailer_dsn'] = [
  'scheme' => 'smtp',
  'host' => 'localhost',
  'port' => 1025,
];

OK, thanks for the details.

But there are several reasons for not committing system commands in the config files : these data could be sensitives, only known to sysadmin, changed frequently or dynamically generated on deployment.

Why not using a keyed array?
Ex:

$settings['mailer_sendmail_commands'] = [
   'default' => '/usr/local/bin/sendmail',
   'Mail pit' => '/usr/local/bin/mailpit sendmail -t --smtp-addr 127.0.0.1:1025 -t',
   'sendmail (internal)' => '/usr/local/bin/intermail -c config1.conf',
   'sendmail (external)' => '/usr/local/bin/extenmail -c config2.conf',
];

That way you can safely commit the key(s) in the config (and even avoid using config split).

If you're agree you could put back this issue status to "Feature request", otherwise feel free to close it.

I notice that the problem is solved if you install it manually instead of using config import, the table is now present.
Maybe there something missing in the install process or config export, hook_entity_base_field_info seems not to be executed by config import ?

For the record you have to uninstall the module by hacking thecore.extension config and install it again manually:

drush eval "\$module_data = \Drupal::config('core.extension')->get('module'); unset(\$module_data['menu_item_role_access']); \Drupal::configFactory()->getEditable('core.extension')->set('module', \$module_data)->save();"
drush cr
drush en menu_item_role_access -y


drush sqlq "SHOW TABLES LIKE '%menu_item_roles';"
menu_link_content__menu_item_roles
menu_link_content_revision__menu_item_roles

Hi,

I reproduce with 8.x-2. + Drupal 10.4.3

drush pmu menu_item_role_access

In ExceptionHandler.php line 56:

  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'db.menu_link_content_revision__menu_item_roles' doesn't exist: SELECT 1 AS "expression"
  FROM
  "menu_link_content_revision__menu_item_roles" "t"
  WHERE "menu_item_roles_target_id" IS NOT NULL
  LIMIT 1 OFFSET 0; Array
  (
  )


In StatementWrapperIterator.php line 113:

  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'db.menu_link_content_revision__menu_item_roles' doesn't exist



Failed to run drush pmu menu_item_role_access: exit status 1

I also have a error on menu item creation:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'db.menu_link_content__menu_item_roles' doesn't exist: INSERT INTO "menu_link_content__menu_item_roles" ("entity_id", "revision_id", "bundle", "delta", "langcode", "menu_item_roles_target_id") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5), (:db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => 1 [:db_insert_placeholder_2] => menu_link_content [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => fr [:db_insert_placeholder_5] => administrator [:db_insert_placeholder_6] => 1 [:db_insert_placeholder_7] => 1 [:db_insert_placeholder_8] => menu_link_content [:db_insert_placeholder_9] => 1 [:db_insert_placeholder_10] => fr [:db_insert_placeholder_11] => sgami ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Drupal\Core\Database\StatementWrapperIterator->execute() (Line: 44)
Drupal\mysql\Driver\Database\mysql\Insert->execute() (Line: 1400)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->saveToDedicatedTables() (Line: 971)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->doSaveFieldItems() (Line: 718)
Drupal\Core\Entity\ContentEntityStorageBase->doSave() (Line: 486)
Drupal\Core\Entity\EntityStorageBase->save() (Line: 806)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (Line: 354)
Drupal\Core\Entity\EntityBase->save() (Line: 148)
Drupal\menu_link_content\Form\MenuLinkContentForm->save()
call_user_func_array() (Line: 129)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 67)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 597)
Drupal\Core\Form\FormBuilder->processForm() (Line: 326)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 48)
Drupal\Core\Entity\EntityFormBuilder->getForm() (Line: 28)
Drupal\menu_link_content\Controller\MenuController->addLink()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 741)
Drupal\Core\DrupalKernel->handle() (Line: 19)

For the record, menu_item_role_access_entity_base_field_info doesn't seems to create the menu_link_content__menu_item_roles table.

o'briat → created an issue.

There's a missing doc about cache keys (https://md-systems.github.io/drupal-8-caching/#/8/1)

Emphasis that Drupal cache max-age is not related to HTTP's Cache-Control header's max-age directive

The "Configuring the Internal Page Cache module" is misleading, one could think that the cache duration (

cache-control header) is generated to this module.

Either rename this page to "Cache configuration" and explain basic cache settings (browser cache & http headers + Internal Page Cache + Dynamic Page Cache) or rename and move this section to a new page.

Also clearly states that this module is the "poor man varnish/cdn"

Add debugging info

The new page still needs to appear in the parent page menu : Installing Drupal →

That seems great, I'll give it a try

Replace link  to old PDO Drupal 7 with php.net

This page is not clear and use very confusing terms specially about "pdo" : that is both  "mandatory" or "to be uninstall".

The page should make it clear for each database type: the minium modules to enable (dot so) in the php.ini file and how to install the matching module for each OS.

Remove python extension suggestion

Hi,
We could use a link to your .vscode folder  https://github.com/webksde/ddev-vscode-devcontainer-drupal-template/tree...

We could start a suggestions "curration" here in the comments, honestly I'm new to Vscode so I may not have picked the correct one.

Here's "yours"  :

    "cvergne.vscode-php-getters-setters-cv",
    "DEVSENSE.composer-php-vscode",
    "dmitrydorofeev.empty-indent",
    "esbenp.prettier-vscode",
    "jgclark.vscode-todo-highlight",
    "mblode.twig-language-2",
    "neilbrayfield.php-docblocker",
    "streetsidesoftware.code-spell-checker",
    "stylelint.vscode-stylelint"
    "ValeryanM.vscode-phpsab",

Here's "mine"  :

    "donjayamanne.githistory", =>  git tool
    "duboiss.sf-pack" => symfony pack (could be useful?)
    "eduardgenzora.symfony-service-locator", => does not work, do  not ananlyse the services.yml or routing.yml files :(
    "formulahendry.vscode-mysql", => add a mysql admin tool to vscode
    "george-alisson.html-preview-vscode", => html preview
    "ikappas.composer", > recommanded by this page, should  be replace  by DEVSENSE.composer-php-vscode (more popular) ?
    "marcostazi.vs-code-drupal" =>  Drupal Syntax Highlighting
    "mikestead.dotenv" => could be useful when using dovenv files
    "mohd-akram.vscode-html-format", => to be challenged
    "nadim-vscode.symfony-code-snippets",
    "phproberto.vscode-php-getters-setters", => to be challenged
    "redhat.vscode-yaml", => Yaml code
    "stanislav.vscode-drupal", => recommanded by this page
    "thenouillet.symfony-vscode",

Remove non "drupal" extensions

So there's no legal issue any more, the core DSFR itself is not "taint" by the GPL?

The limitation about using this theme on a non French government website is still valid?

It's not only about formatting but cover the general Vscode Setup, including debugging and all the useful extensions that could help Drupal development.

Here's my git repo : https://github.com/obriat/vscode4drupal

The extensions file does not install them automatically but provide suggestions (notifications and a category in the extensions UI)

A full .vscode folder with all json files (settings, extensions, launch) should be provided (zip to download or git repo), no?

Workspace recommended extensions

It should be made clear that the {machine_name}.info.yml filename is the source of  the module machine name

Advice on permission naming

This page needs to explain how to setup the Drupal configuration export/import, specially if one uses the suggested config : ../sync

Use ASCII tree

Great idea, writing log to file is quite slow.

But using the Symfony bundle seems a bit overkill, no ? The logging process should the lighter possible, just using a simple guzzle call, a non blocking one if possible (https://drupal.stackexchange.com/questions/296743/non-blocking-http-requ...).

The current "processing rate" could also be displayed to add a clear message, ex:

"This can happen when no processors are clearing your queue, or when queueing outpaces processing. Please first solve the structural nature of the issue by adding processing power or reducing your queue loads. Empty the queue to unblock your system. The current processing rate (@current_rate clearing requests/s) is lower than the queue growth one (@growth_rate new item to clear/s)."

Add direct link to Drupal8+ doc

The most elegant way will be a form with a check box to enable or not the auto detection based on reverse proxy. If uncheck, IPs should be input in a text field.

Why not take example on the ZeroConfig Purger?
It does multiple call based on the reverseProxies list:
https://git.drupalcode.org/project/varnish_purge/-/blame/8.x-2.x/src/Plu...

Does the vcl example file for zeroconfig does the same thing ? It works for me since ages.

https://git.drupalcode.org/project/varnish_purge/-/blob/8.x-2.x/zeroconf...

The problem could also legitimately occurred when massive import/update batch are executed regularly.

The module could provide an option that purge all cache (drush p:invalidate everything -y) and empty the queue (drush p:queue-empty) ?

Format code. The "number 3" needs more info: add details  about file registration (linked to node id or module, name, ...)

So, what's left :

• Force HttpOnly to false and remove it from the admin form
• Update the MR with the minor fixes from 3308456-30.patch #30
• Change the MR target branch to 2.x (or create a new one if this fix should still apply to 1.x)

Split checks to match methods

Add precision to the check part

What are the prerequisites for using transactions? For example, does the binlog needs to be enable?

Yes, I know but with just two carriage returns all the module now complies with the Drupal standards. If you think it's not needed, I'll revert it.

Phpcs fixed, I also added two minors fixes on CronTest.php

The patch for #3471571 is now live on an production drupal. So far it seems ok.
Could some else tests it and marks the issue as RTBC if ok and close this one at the same time?

The patch is live on an production drupal. So far it seems ok
Could some else tests it and marks this issue as RTBC if ok?

o'briat → created an issue.

o'briat → created an issue.

Add warning abour emergency level + rsyslog

The error is logged with an emergency level (!) which seems a bit overkilled for a simple ban call.

And by the way, Emergency level + rsyslog leads to sending message to all users shells.

*.emerg :omusrmsg:*

https://www.rsyslog.com/doc/configuration/modules/omusrmsg.html#write-em...

I don't remember why but I added this file to my "packaging exclusion list" (along with install.php, readme,...).
Can someone explain me what's the use of this file on a packaged deployed website (updates done only with composer)?

After digging a bit more into pruger I found that "Runtime measurement" is a "duration autodetection" feature :
https://git.drupalcode.org/project/purge/-/blame/8.x-3.x/src/Plugin/Purg...

So it's ok to have errors that pop during the first calls.

IMHO, this info should ba added to the help text of the field and the timeout exception should be handle specifically , do not log under a high value (30s or a multiple of the basic value) or not with the emergency status.

o'briat → created an issue.

Sorry for the mess, I 'll have to dig a but more, all I recall is that #3293026 doesn't fix a specific case.
I'll try to make a summary

o'briat → created an issue.

If your Varnish servers are load balanced, it should easier to add a specific rules to your load balancer (filtered on drupal IP + PURGE/BAN method) so it send the request to both all Varnish instances.

So you just need to set the inbound LB IP.

My 2c:

I wonder of this whole cache tag invalidation is not overkill?

It's a mess to configure: adding and setting two modules and purgers, managing a specific queue with processors like frequent cron or (worst) a later runtime, the queue could explode if you massively/frequently update contents, managing a specific varnish config (good luck if it's load balanced), ...

It add a real complexity to your website architecture.

I simple varnish configuration that cache your anonymous pages for 5 should be enough in most of the cases. If your website needs to display fresh information in real time: use ajax or push events API.

The MR is already done, I just added the patch so it can be added safely into compose.json (the MR link could be altered by additional commit, it's still an global issue: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → ). I'll edit the message for clarity.

Please test the MR or patch

Here the MR and the patch.

The new token should only be used with the content type application/json to avoid escape chars. Maybe this behavior should be extended to all purger's body?

It should also fix, the todo in buildFormTokensHelp about token auto detection (there a lack of doc bout the core token api).

IMHO none of the tokens should be escaped.

The problem occurs in \Drupal\purge_purger_http\Plugin\Purge\Purger\HttpPurgerBase::getOptions, this function should use \Drupal\Core\Utility\Token::replacePlain instead of \Drupal\Core\Utility\Token::replace.

I stumble on the same issue when trying to provide a new token (see related issue), in my patch I propose a "safe way" by switching to replacePlain only if $this->settings->body_content_type === 'application/json'.

o'briat → created an issue.

o'briat → changed the visibility of the branch 3373411-how-to-purge to hidden.