This was fixed while fixing issue 3439306 ✨ Provide public method to retrieve data only Fixed .
https://git.drupalcode.org/project/orange_dam/-/merge_requests/58
gpietrzak → credited apotek → .
gpietrzak → credited apotek → .
gpietrzak → credited apotek → .
gpietrzak → credited apotek → .
gpietrzak → credited apotek → .
gpietrzak → credited apotek → .
gpietrzak → credited apotek → .
Just reporting back, in case anyone else is experiencing this issue:
The status check fails in two odd cases.
Case 1: Where the exported config set and the installed config set are identical except for the order of some of the properties on the XML nodes. The value of the properties and the properties themselves are identical, just in a different order. So functionally, the XML is identical but the status report will still trigger a warning, which is unwarranted in this case.
Case 2: In my QA environment, I have configured the solr server to symlink /var/solr/data/mycore/conf to a mounted solr-conf directory. The contents of that mounted solr-conf directory consists exactly and exclusively of the config-set I exported from that very server before rebuilding it. So I downloaded the config-set from the QA server. I expanded the zip file into my solr-conf directory, rebuilt my QA solr server, and I still get the status report warning. If I shell into my QA solr server and compare the files in /var/solr/data/mycore/conf to the files I get when downloading the zip, they are identical.
I can only conclude that the comparison in this function is a little off and I advise people to ignore the warning if they have aleady made a good faith effort to resolve it with a new config.zip retrieval.
This was handled in https://www.drupal.org/project/orange_dam/issues/3441899 ✨ Make OrangeDamApi::getKeywords a pure get function Fixed
Already done. Though we could start planning version 3 :)
This patch seems to work as advertised. I don't know why this issue was taken from needs review to needs work without feedback. What is the patch lacking? Can that be made explicit? Putting this back to needs review.
I am also confused by this. If hook_install/uninstall are documented as being the way by which we can install custom entities or custom fields on entities (and remove them), but we are being told *not* to make such changes in these hooks during a config:import (which is the way most sites these days install and uninstall modules, not via explicity drush commands or the web gui), then it seems we are unable to have our custom fields or entities installed as part of a site deployment.
Or are hook_uninstall() and hook_install() run twice? Once during config:import and once during some other part of the deployment process?
Very confused here as this requirement to not make changes to entities in hook_install seems to run counter to the whole point of the hook.
Full migration tests were run. Everything checked out.
Thank you @markdorison and @rajan-kumar2026. In addition to the suggestions Mark is making, I think the point of this ticket is to figure out if we want to support Drush 12 or not. Currently, our drush commands are not compatible with Drush 12. So if we are going to add
"drush/drush": "^11 || ^12 "
we are basically saying we support drush 12, when we _don't_ support drush 12 (yet).
If we are going to add `^12` we will have to rewrite a lot of code in this module so that it can still output to the console. It's a big job.
apotek → created an issue.
apotek → created an issue.
I feel it might be worth adding a section on limitations to this. For example, you can't set the priority of your overrides. A use case I tried to solve for might illustrate: Writing a drush command where one of the options passed to the command is intended to override the configuration, but just for the run of that drush command. I can create the override, and add the override to my config factory, but (and here come the limitations):
- The override does not change the global state of the configuration unless it is loaded as service. If you are setting the override in code, only your exact class and following code has access to the override, in which case you are getting no benefits from doing the override at all. In other words, you _can_ make an override dynamically, but it is only available to your current execution scope. A config factory in another class will not pick it up.
- You cannot override any config that might be set in settings.php. Since you can't control what users might set there, you effectively cannot have any assurance that your override, even when loaded as a service, will have any effect at all.
In case of failed validation, the documentation here, as elsewhere, explains that
> by default the return will be a 404 page.
but I can find no documentation on how to change that default response type. The structure of routes → overview page mentions nothing to this effect.
In the case of a malformed value, for example, a developer might prefer to follow the standard and issue an HTTP Bad Request (400) response instead of a possibly misleading 404 Not Found.
But I can see no way to do this.
Suggestions:
1) If there *is no way* to change the response type, the documentation should be updated to say:
- In the case the validation fails, by default the return will be a 404 page.
+ If validation fails, a 404 Response will be issued. If you want to respond with another HTTP response, you cannot use this form of validation and will need to implement your own within your controller.
2) If there *is* a way to change the default response type, can we add that documentation here?
Thank you.
Following the exact instructions on the home page (for version 5). I put this in my composer.json libraries section:
{
"type": "package",
"package": {
"name": "enyo/dropzone",
"version": "5.9.3",
"type": "drupal-library",
"dist": {
"url": "https://github.com/dropzone/dropzone/releases/download/v5.9.3/dist.zip",
"type": "zip"
}
}
}
I run `composer require drupal/dropzonejs enyo/dropzone` and I get the exact output mentioned by @kopeboy in #4.
My previous installation had `^5.7` as the enyo/dropzone constraint. I also tried requiring "enyo/dropzone:^5.9" and the same response came out: "Root composer.json requires enyo/dropzone ^5.9, 5.7.2, found enyo/dropzone[5.7.2] but it does not match the constraint"
In the end, while I can update drupal/dropzonejs to latest, I am unable to update enyo/dropzone to 5.9.3 and am stuck on 5.7.2. This is not breaking the site, but obviously it is less than ideal. What is interesting is that dependabot keeps trying to update enyo/dropzone to 5.9.3 and I get pull requests for an updated composer.lock with the new 5.9.3 path in it, but my builds fail of course when my CI tries to build the dependabot PRs.
Seems like there is a hangup somewhere that I certainly cannot identify. --with-all-dependencies does not fix this, nor does a relaxation of minimum stability.
Thank you @fjgarlin, @berdir for your patient replies. Many of us mortals do not have the ability to monitor slack daily and stay on top of every META issue. It's hard enough to just (fail) at keeping up with bug fixes and feature requests. I accept that the cost of not participating on Mt Olympus is that our fate must be handed to us.
I just feel in the past year that the balance of work done to develop actual features or fix bugs is reaching less than half, and the other more-than-half is just keeping up with deprecations, Symfonyisms, phpstan and linters. That's what I am seeing in the issue queues. Thanks again for the responses. I'll try to stay on top of META developments in the future.
Is there anyone who takes the time to assess how these decisions affect contrib developers. A person makes a suggestion, there's no meaningful debate, and the next thing you know, every maintainer of every contrib module now has to spend time making cspell happy. When code linters came out I was an early adopter. Now I see merge request after merge request that add nothing to the functionality of the modules, but merely strive to keep them compliant with the latest enforcement tools. And... in the comments on contributed fixes, more is made of an linter warning on a yml indentation, that on how the code is actually working.
I am waiting for someone to roll out a linter that enforces the correct use of the oxford comma.
Where is this stuff being talked about? And is it ok to push the same core procedures out onto all contributed module developers without any meaningful debate at all? (I apologize if there is such a forum, in which case, I would love a link to that discussion).
Merge request is up:
We still get this notification:
[warning] Unable to find migrated content having “JF11978345” as a source id while processing the deletion queue.
But the queue item is released from the queue….
select * from queue where data like ‘%JF11978345%’;
[nada]
So it will not be run again and again ...
apotek → created an issue.
Added relationship to issue #3084967, which has the solution.
> I can reproduce the issue, but setting "Needs work" since there's open threads on merge request.
Keeping this as reviewed and tested. I am not sure why gitlab says there are three unresolved threads. The "resolve thread" buttons are not visible, so while it indicates that all threads are unresolved, it appears that they actually were resolved at some point in time. In any case, all questions have been resolved with decisions or changes.
This looks good to go.
Thank you @markdorison for helpful reviews!
I think the thing to do here is to close this as fixed and we'll open more targeted requests for where specific options are misaligned and where that causes the most friction. This was a win, but not complete. But completing it can't be a top priority now, and we should close this as an improvement that is good enough that we can later iterate over.
perhaps they have to live in the EventSubscriber namespace of a module?
Yes, I was thinking so. Just like the route discovery ticket requires the Controller/ namespace.
Symfony has an AsEventListener attribute that we should consider using first, so Symfony developers don't have anything new to learn. https://symfony.com/doc/current/event_dispatcher.html#defining-event-lis...
That is a great reference! I hope you don't mind me adding that to the issue description! Thank you!
apotek → created an issue.
apotek → created an issue.
Thank you for your valuable input @Nikolay Shapovalov. I have unassigned this issue and now let's see if the maintainers pick it up.
@apaderno I wasn't really ready for this to be reviewed, but your feedback is valuable. Unfortunately, the majority of my commits are not visible in gitlab since they were made in github before the module was contributed.
Since I can't contribute to existing modules, I have started my own project and will update this ticket when I am done with it. Thank you.
> Needs review is the status used when the project is ready for review.
Yes, @vishal.kadam, thank you for the reminder, but I kept it as `active` on purpose since I wasn't ready for this be reviewed. Thanks again :)
Thank you for your review @nikolay-shapovalov. I have added the suggestions you made. I have left unresolved your question about changing
- use Drupal\Dfp\Entity\Tag;
+ use Drupal\dfp\Entity\Tag;
This was one, very easy to resolve issue highlighted by phpstan. If you want me to remove it, I will.
> I wasn't able to find "starter phpstan configuration" in the MR.
It is in the `phpstan.neon` file I added. I have changed the title as you suggested.
I have started the application process → . Thanks everyone.
@gpietrzak Thank you for contributing!
Strengths
1. The project has a well defined mission, a good intro page, and the module is mature, in the sense that it includes a correct composer.json file, a good MODULE_info.yaml file, and the code layout matches what is expected of a Drupal module.
2. Correct use of t() functions.
3. Demonstrates awareness of how to use routes rather than hard-coding paths throughout code:
Url::fromRoute('advanced_mautic_integration.admin_settings_form') Has correctly implemented a module links.yml file and routing.yml file.
4. Commenting: Fulfills baseline commenting requirements both in .module and classes, and adding thoughtful and simple explanations where appropriate without adding clutter.
5. Services. Code demonstrates correct use of DI for loading services in classes, and \Drupal::service() within .module code.
advanced_mautic_integration.visibility:
class: Drupal\advanced_mautic_integration\VisibilityTracker
arguments: [ '@config.factory', '@plugin.manager.condition', '@context.handler', '@context.repository' ]
6. Awareness of when adding cache tags is necessary.
$cache->addCacheableDependency($bubbleable_metadata);
$cache->applyTo($page);
7. Attention to detail: Module even implements hook_help(). Also writes Interface classes for his custom services rather than just a service with no Interface defined for it. A+
8. Demonstrates understanding of how to set up libraries. I would encourage adding a `version: 1.x` key to your library yaml in order to help aggregation figure out when cache needs to be changed.
tracking_events:
version: 1.x
js:
js/tracking_events.js: {}
dependencies:
- core/drupal
- core/once
Improvements
1. In MauticScript::getScript() I would encourage re-thinking the generation of the script in PHP. Instead I might suggest adding the dynamic part of the script to Drupal settings, and adding the static part of the script to your library yaml.
2. Since your module has configuration, I would encourage the use of a `config/schema/advanced_mautic_integration.schema.yml` file, and a `config/install/advanced_mautic_integration.settings.yml` file. This way, when the module is enabled, default values and data types for all the expected config can be counted on to exist, and you don't need to validate every time you call `$config->get()`. For example: 'trackPageviews' => $config->get('track.pageviews') ?? FALSE, just becomes 'trackPageviews' => $config->get('track.pageviews'); because you have a schema with data types for all your configuration variables, and you have already defined default values in your install :)
3. Wondering if some of your `private` vars should be `protected`
public function __construct(
private readonly ConfigFactoryInterface $configFactory,
private readonly MauticApiWrapperInterface $apiWrapper,
private readonly AccountProxyInterface $currentUser,
private readonly RequestStack $requestStack,
) {}
or is the expectation that UserSynchronizer should be extensible and these should be visible to any child classes? I don't know what the plans are but I would tend to default to the most restrictive unless you need something more open. Same thing for the other classes where private is used, such as the VisibilityTracker class.
4. On your project page, you say
Unfortunately, the latest 3.1.0 release of the library has problems with Psr/Log. Therefore, I had to use the main branch. I will change it back when a new version of the library is released.
Would you be able to work around this in your wrapper class by decorating whatever method in there has the issue with Psr/Log? Do you have to use the raw Mautic API library?
5. Code style fixes: Using branch 1.0.x I see a few code style issues in the javascript library. You may also want to run this by eslint.
$ phpcs --standard=Drupal,DrupalPractice advanced_mautic_integration
FILE: ...html/web/modules/contrib/advanced_mautic_integration/js/tracking_events.js
--------------------------------------------------------------------------------
FOUND 8 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
48 | ERROR | [x] Expected 1 space before "?"; 0 found
48 | ERROR | [x] Expected 1 space after "?"; 0 found
48 | ERROR | [x] Expected 1 space before ":"; 0 found
48 | ERROR | [x] Expected 1 space after ":"; 0 found
56 | ERROR | [x] Expected 1 space before "?"; 0 found
56 | ERROR | [x] Expected 1 space after "?"; 0 found
56 | ERROR | [x] Expected 1 space before "?"; 0 found
56 | ERROR | [x] Expected 1 space after "?"; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
Time: 148ms; Memory: 6MB
So congratulations on the short list of code style issues flagged! Nicely done. This module is impressively implemented. And thank you for showing me this:
$visibility_collection = new ConditionPluginCollection($this->conditionManager, $visibility_config);
I was not aware you could do that.
I have resolved all issues raised by @nikolay-shapovalov so am switching this back to Needs Review. gitlab-ci is working and reporting back helpful results.
A more thorough fix for this is in this issue/merge request that has also been sitting in the issue queue for a while.
https://www.drupal.org/project/fastly/issues/3253019 🐛 VCL upload errors don't appear on the form Needs review
Updated title to reflect focus on this issue being the coding standards and letting the initial addition of gitlab-ci work be addressed separately https://www.drupal.org/project/dfp/issues/3416546 📌 Add gitlab.ci Needs work
Should we create a separate issue just for the gitlab work (which is complete) and at least be able to get that ported into any active branches?Yes, this is good idea to create separate issue.
And separate issue to fix phpunit tests can be created as well.
@nikolay-shapovalov I have created a separate issue 📌 Add gitlab.ci Needs work just for the gitlab-ci.yml starter file, so we can get that in and start getting feedback on merge requests, even if tests are allowed to fail.
https://www.drupal.org/project/dfp/issues/3416546 📌 Add gitlab.ci Needs work
And as you said, moving the test fixes into a separate issue would also be good to do... Thank you.