Account created on 9 January 2018, over 6 years ago
#

Merge Requests

More

Recent comments

🇵🇱Poland wotnak

There doesn't currently seem to be a config option for that. Both hidding toolbar and linking logo to drupal.org is hardcoded.

It however can be fixed with a bit of custom code. Logo link is hardcoded in the page--mercury-editor template that can be overriden and showing admin toolbar is controlled by the `_hide_admin_toolbar` route option that can be altered . I'm using following route subscriber to do that:

class MercuryEditorRouteSubscriber extends RouteSubscriberBase {
  /**
   * {@inheritdoc}
   */
  protected function alterRoutes(RouteCollection $collection): void {
    // Show admin navigation on Mercury Editor route.
    $mercuryEditorRoute = $collection->get('mercury_editor.editor');
    if ($mercuryEditorRoute !== NULL) {
      $mercuryEditorRoute->setOption('_hide_admin_toolbar', FALSE);
    }
  }

}

It works quite well for the new admin navigation (combined with changes from https://www.drupal.org/project/mercury_editor/issues/3449328 Hide new drupal admin navigation on mercury editor routes Needs review ), not sure how it will look with other admin toolbars but it should make them show up.

🇵🇱Poland wotnak

Created merge request that adds support for removing the new admin navigation for the routes with the _hide_admin_toolbar option set to true, simillary to how other toolbars are removed.

With this change Mercury Editor page renders corectly without admin navigation toolbars:

🇵🇱Poland wotnak

Marking as RTBC. Changes from https://git.drupalcode.org/project/vite/-/merge_requests/12 worked for us in internal and client projects without any problems.

🇵🇱Poland wotnak

Changing to needs work as it is missing documentation of the new devDependencies option.

🇵🇱Poland wotnak

Change looks good. It seems that treating css as chunks worked in earlier Vite versions but was never the intended approach. https://vitejs.dev/guide/migration.html#corresponding-css-files-are-not-...

🇵🇱Poland wotnak

Give credit to developer who worked on the module.

🇵🇱Poland wotnak

Initial module release created: 1.0.0 .

🇵🇱Poland wotnak

Give credit to developer who worked on implementing module functionality.

🇵🇱Poland wotnak

Initial module release was published: 1.0.0 .

🇵🇱Poland wotnak

Credit developers who worked on initial module implementation.

🇵🇱Poland wotnak

Local tasks (tabs) are by default cached per route, user.permissions and specific entity, for example test project and crop plan I was testing this changes have following CacheableMetadata

Using hook_menu_local_tasks_alter to remove tab under specific conditions is safe and there's even an example in drupal core https://git.drupalcode.org/project/drupal/-/blob/820d76b2c71d22132796969....

But I think you're right that replacing _entity_bundles restriction with bundle key in route param would be in this case a better approach. Updated the MR to use this approach, it seems to work well.

🇵🇱Poland wotnak

Created merge request with hook_menu_local_tasks_alter implementation that removes project logs tab for all route matches that don't have project plan in params. This results in 'Logs' tab being visible only on project plans and not on other plan types.

🇵🇱Poland wotnak

wotnak made their first commit to this issue’s fork.

🇵🇱Poland wotnak

A quick fix could be to just check in the custom install task if $install_state['parameters']['existing_config'] is true and if so skip automatic installation of base modules.

🇵🇱Poland wotnak

This is caused by a change in Vite 5. https://vitejs.dev/guide/migration.html#corresponding-css-files-are-not-...
In earlier versions css files were added as separate chunks, in Vite 5 for js entrypoints they are only listed in the js chunk under css key.

Manifest generated for a js file that imports css file:

  • on Vite 3 and 4
      {
        "src/main.css": {
          "file": "assets/main-98fde70f.css",
          "src": "src/main.css"
        },
        "src/main.js": {
          "css": [
            "assets/main-98fde70f.css"
          ],
          "file": "assets/main-690fc99d.js",
          "isEntry": true,
          "src": "src/main.js"
        }
      }
      
  • on Vite 5
      {
        "src/main.js": {
          "css": [
            "assets/main-gINCZU8i.css"
          ],
          "file": "assets/main-6jBz11gc.js",
          "isEntry": true,
          "src": "src/main.js"
        }
      }
      

Since we already support both Vite 3-4 and Vite 5 in other places (manifest location, dev server response codes) we should also ensure support for both versions in this case.

🇵🇱Poland wotnak

Change looks good, will merge it.

But first, I think it would be also good to provide an option to enable vite in all components/libraries of the theme/module right in the theme/module, so that vite configuration is contained in the theme/module it applies to and changes to settings.php aren't required for the theme/module to work.

Thinking of adding vite section to the .info.yml file that will allow enabling vite in all sdc components and/or all libraries defined in .libraries.yml, something like:

name: My theme
[...]
vite:
  enableInAllComponents: true
  enableInAllLibraries: true
[...]
🇵🇱Poland wotnak

wotnak made their first commit to this issue’s fork.

🇵🇱Poland wotnak

MR !5248 adds vincentlanglet/twig-cs-fixer instead of friendsoftwig/twigcs about which is this issue.

That being said, friendsoftwig/twigcs maintainer is looking for someone to take over the maintainership and considers putting the project in the EOL status if no one will step up.
https://github.com/friendsoftwig/twigcs/issues/304

One alternative they point to is vincentlanglet/twig-cs-fixer (which MR !5248 adds).
Personally, I also prefer vincentlanglet/twig-cs-fixer since it also includes automatic fixes. We already using it for some time at work in Drupal based projects, and it works great.

🇵🇱Poland wotnak

Available in the 1.1.1 release.

🇵🇱Poland wotnak

Available in 1.1.1 release.

🇵🇱Poland wotnak

Committed SDC integration in https://git.drupalcode.org/project/vite/-/commit/4fee955a75ceac335adfe82.... Instructions are in the README https://git.drupalcode.org/project/vite#sdc-integration.

Basically for SDC is required the same configuration as for normal theme/module libraries, only in libraryOverrides in component definition instead of in .libraries.yml.

🇵🇱Poland wotnak

Change looks good. The only thing to fix is one issue reported by phpstan (you can see it in the merge request ui).
The in_array function should be called with the third parameter set to true to strictly compare values, including checking their types. https://www.php.net/manual/en/function.in-array.php#refsect1-function.in...

🇵🇱Poland wotnak

Added props default value functionality implementation in https://git.drupalcode.org/project/drupal/-/merge_requests/5353. It passes test modified by justafish.
I implemented it in `sdc_additional_context` twig function that is executed in ComponentNodeVisitor and already used to add things to component context. The implementation loops through component props and if a given prop has a default value set and doesn't already have a value in the current context, the default value is added to the context (which makes it available in the component template).

🇵🇱Poland wotnak

wotnak made their first commit to this issue’s fork.

🇵🇱Poland wotnak

The name of the new settings is rather generic but is only used for oEmbed. Should the name be changed to reflect that?

It is not used only for oEmbed. The primary use of `http_client_config` is in ClientFactory where it allows overriding the default http client settings, including the timeout (which by default is set to 30s).

The description in default.settings.php should probably also mention that changing this setting will impact all code that uses the http_client service without providing custom timeout explicitly.

Maybe a better way to approach this would be to provide a separate setting for the oEmbed ResourceFetcher timeout, since from what I understand the 5s limit was added in https://www.drupal.org/project/drupal/issues/3202145 specifically to stop using the default global timeout value.

🇵🇱Poland wotnak

It would be nice to implement this in a general way to allow for including potential similar scripts for other frameworks.
Maybe we could add a concept of devDependencies to the vite section of library definition. It could allow providing a list of other libraries like in the default 'dependencies' section, but they would be attached only when running in the vite development mode.
Then for this specific use case, in this module we could define a vite/plugin-react-refresh library that includes the script. Then in a theme/module where vite-plugin-react is used, in the library definition this library could be added to the devDependencies:

react-block:
  vite:
    enabled: true
    devDependencies:
    - vite/plugin-react-refresh
  js:
    ...

The plugin-react-refresh script will also need the current vite dev server url. Since it can be different for every theme/module passing it through drupalSettings won't really work because the script won't know which url to use. Maybe we could pass it to all devDependecies through data-vite-dev-server-url attribute then in scripts it could be fetched by accessing attributes of document.currentScript.

🇵🇱Poland wotnak

Added two last comments in the merge request, after resolving them this will be ready to be merged.

🇵🇱Poland wotnak

Added some comments in the merge request.
WMS also allows two optional parms `VERSION` and `STYLES`, some WMS services even require them to correctly load maps. To make the WMS support more complete, it would be nice to also add an option to provide these two params in the custom map layer configuration.

🇵🇱Poland wotnak

Related to https://www.drupal.org/project/drupal/issues/3365480 🐛 [SDC] Improve error handling during prop validation errors Fixed where similar problem was fixed for passed props validation.
Same fix of adding `Constraint::CHECK_MODE_TYPE_CAST` as a third argument to `$this->validator->validate()` applied at https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/sdc/s... should help. It will validate schema using loose type checking https://github.com/justinrainbow/json-schema/blob/master/src/JsonSchema/... that will treat empty and associative php arrays as valid objects.

🇵🇱Poland wotnak

You already created one issue for this project with exactly the same content and patch.
I replied in the original issue 🐛 All dependencies must be prefixed with the project name, for example "drupal:" Closed: works as designed why omitting project name is in this case intentional workaround and won't be fixed: https://www.drupal.org/project/farm_map_custom_layers/issues/3367910#com... 🐛 All dependencies must be prefixed with the project name, for example "drupal:" Closed: works as designed

🇵🇱Poland wotnak

As stated in the comment right above the dependency declaration, omitting the project name is an intentional workaround for how packages.drupal.org doesn't handle declaring a dependency on a module from a distribution very well. As seen in https://www.drupal.org/project/farm_forest/issues/3190919 this is a workaround suggested by one of the maintainers of packages.drupal.org .
Also, if packages.drupal.org would handle it right, the dependency with project id would have to be `farm:farm_map`, the standalone farm_map project is no longer maintained, and the version updated to farmOS v2 (D8+) is now included directly in the farmOS distribution.

🇵🇱Poland wotnak

Configuring domain solves the problem of binding ports directly, but it is still an additional configuration step that could be skipped by proxying requests to vite dev server. Also, when running multiple vite dev servers simultaneously inside single project (for example in a multisite setup for common base theme and specific site theme or maybe for a theme and a decoupled react based block) you would need to configure multiple domains pointing to different ports.

🇵🇱Poland wotnak

One possible approach to allow not using the module in production while retaining all the advantages of integration with vite manifest.json could be to add an optional build step to `.libraries.yml`. So for development you would do everything the same as now, but when you want to deploy changes to production you would build assets using vite and then execute a build step for `.libraries.yml` that would output rewritten libraries (which is done by default in production) back to the `.libaries.yml` file.

🇵🇱Poland wotnak

It seems that these changes:
- require manifest.json to be generated for dev mode,
- require vite to be configured to not use hashes in built file names,
- with the module disabled in production (which is the motivation for these changes) it requires to manually set all built chunks/dependencies declared in manifest.json directly in .libraries.yml.

Overall these changes break current functionality of the module, require specific vite configuration and require manual .libraries.yml modifications that currently are done automatically.

I don't see a way to merge this as a default behavior, it would need to be behind some setting/flag that needs to be enabled to switch to proposed behavior. Some documentation that would list caveats/requirements to work in this mode should also be added before possible merge.

Also running phpcs and phpstan with configurations provided in the project repo results in the following new errors:

www-data@172775eb8550:~/drupal/web/modules/contrib/vite$ vendor/bin/phpcs .

FILE: /var/www/drupal/web/modules/contrib/vite/src/Manifest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 128 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: /var/www/drupal/web/modules/contrib/vite/src/Vite.php
----------------------------------------------------------------------------------------------------------------------------
FOUND 13 ERRORS AFFECTING 13 LINES
-----------------------------------------------------------------------------------------------------------------------------
 195 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 203 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 220 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 228 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 251 | ERROR | [ ] Missing short description in doc comment
 252 | ERROR | [ ] Missing parameter comment
 253 | ERROR | [ ] Missing parameter comment
 254 | ERROR | [ ] Missing parameter comment
 255 | ERROR | [ ] Description for the @return value is missing
 257 | ERROR | [x] There must not be a space before the colon in a return type declaration
 258 | ERROR | [x] Opening brace should be on the same line as the declaration
 262 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 267 | ERROR | [x] Expected newline after closing brace
-----------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------------

Time: 99ms; Memory: 12MB

www-data@172775eb8550:~/drupal/web/modules/contrib/vite$ vendor/bin/phpstan analyse .
Note: Using configuration file /var/www/drupal/web/modules/contrib/vite/phpstan.neon.
 7/7 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ------------------------------------------------------------------------------------------------------------ 
  Line   src/Vite.php                                                                                                
 ------ ------------------------------------------------------------------------------------------------------------ 
  257    Method Drupal\vite\Vite::getNewLibraryPath() never returns null so it can be removed from the return type.  
  267    Else branch is unreachable because previous condition is always true.                                       
 ------ ------------------------------------------------------------------------------------------------------------ 
🇵🇱Poland wotnak

No, didn't yet get to preparing them. The starting point would depend on what you want to do. Use vite for building theme styles/scripts? Maybe for a custom decoupled block? Configuration for integration with vite dev server will also depend (at least until https://www.drupal.org/project/vite/issues/3340765 Proxy requests to vite dev server Active is implemented) on your development environment setup.

On GitHub there seems to be a project that uses this module for a custom theme with styles written in scss https://github.com/bronisMateusz/visualize, maybe it can be of some help.

I also created a new issue https://www.drupal.org/project/vite/issues/3349311 📌 Example configurations/starting points Active to track creating some examples. If you have some specific use case/configuration you would want to see an example of, you can describe it there, and I will try to implement it on the weekend.

🇵🇱Poland wotnak

The module has two primary purposes:
- integration with vite dev server for development,
- and integration with vite asset manifest for production.

The proposed resolution would only work if assets built by vite would have static names, which is by default not the case (they have dynamically generated hashes appended to the filenames).

The module implements vite backend integration as described on https://vitejs.dev/guide/backend-integration.html#backend-integration.

I don't plan to explore the requested use case. That said, I'm open to accept contributions If someone would want to implement it.

🇵🇱Poland wotnak

Even if we will stick with Drupal PostgreSQL minimum version requirements, it could be nice to add automatic pg_trgm extension installation when PostgreSQL v13 or newer is detected.

🇵🇱Poland wotnak

It looks like the 'trusted extensions' concept was added in postgres 13 https://www.postgresql.org/docs/13/release-13.html#id-1.11.6.15.5.14. Drupal 10 supports postgres versions starting with 12 which in fact requires superuser db account to create extensions.

Since by updating to D10, farmOS v3 will already increase minimum required postgres version to 12 maybe we could consider increasing it to 13 instead which would allow to safely automatically create pg_trgm extension.

🇵🇱Poland wotnak

I think this will largely be a manual step that site admins need to do. We can't really automate it, because CREATE EXTENSION pg_trgm; must be run by a PostgreSQL user with higher privileges than that Drupal database user should have.

https://www.postgresql.org/docs/current/pgtrgm.html

This module is considered “trusted”, that is, it can be installed by non-superusers who have CREATE privilege on the current database.

Which means that pg_trgm should be installable by the standard Drupal db user without any additional privileges.

As for how pg_trgm could be automatically installed in the docker setup, one solution could be to install the extension in the farmOS docker image entrypoint using drush. It has the advantage that it should automatically work for new as well as existing databases. I solved it this way in my custom farmOS docker image https://github.com/wotnak/farmos-docker/blob/main/docker-entrypoint#L12-L26.

🇵🇱Poland wotnak

In this case, not including the project name in the dependency declaration was intentional. It is a workaround for how packages.drupal.org handles or rather doesn't handle well declaring dependencies on modules from a distribution.

https://farmos.discourse.group/t/termination-events/1476/26
https://www.drupal.org/project/farm_forest/issues/3190919

It would be however probably good to add a comment explaining it and find or create an issue for the https://www.drupal.org/project/project_composer to link it in the comment.

Production build 0.69.0 2024