Just noticed the deprecated notice on the module status and found more information in the documentation (maybe a bit more information wouldn't hurt on the project page either).
Anyway, I rolled back some unrelated changes, which hopefully makes reviewing this a little easier. However, with tests out due to the move to GitLab, I have little faith in this actually ever making the finish line...
I updated the MR with the latest changes from #21 and merged in 4.x in the process. I'm looking into the implications of using the AssetsStream wrapper directly... I don't think this will work in the current form with Drupal 9.3, which is currently the oldest core version this module claims to support.
There is a merge request, can that be updated based on the latest changes? Patch workflow is not going to stick around for the long haul, best get to grips with the MR workflow. Also, a quick glance at the patch told me that there are some unrelated code changes in the patch. It is best to limit code changes for an issue to the changes required to solve the issue at hand; it limits the chance of merge conflicts and reduces the number of lines to review.
eelkeblok → created an issue.
What are the security implications? Why is this set to anonymous in the first place? "Works for me" is not really a ringing endorsement for a patch that has security implications :)
Over four years onward, I needed a more nuanced implementation, here's a stab at it. This only deletes flaggings when re-assigning content to the anonymous user (deleting a user will already call the existing pre-delete hook). Also, it checks the number of flaggings to remove to a threshold, and will create a queue to delete the flaggings in batches when it exceeds the threshold. The threshold itself is obviously up for debate, maybe the current 20 is a bit too convervative.
eelkeblok → created an issue.
Slowly chipping away at test failures. Seem to have arrived at a pretty fundamental one; the test is using the public stream wrapper to test if the original file was created. Not confident this is a great idea, maybe we need to introduce an "unwrapped" version of the original stream wrapper for the test.
The config validation that is enabled on tests is tripping up saving the settings form. I tried turning the origin_path into a sequence, but that wasn't the (entire?) solution. FWIW (not sure if this is documented anywhere), I got the config validation to kick in on my dev environment by putting this in an active services.yml (e.g. development.services.yml, *if* that's enabled), to make debugging easier:
services:
testing.config_schema_checker:
class: Drupal\Core\Config\Development\ConfigSchemaChecker
arguments: ['@config.typed', [], FALSE],
tags: [{name: event_subscriber}]I merged in 3.x and one thing led to another... I *think* there was still a problem with trying to add dependency injection to the stream wrappers, unless I misunderstood how things are supposed to work. The stream wrappers end up being registered with PHP as actual stream wrappers, and Drupal/Symfony dependency injection doesn't kick in there, so I was getting errors of the constructor being called without the proper arguments. So, I removed the arguments and replaced them with some methods calling the Drupal global object.
I did my best capturing the spirit of the changes in the 3.x branch, I hope I didn't break anything.
eelkeblok → made their first commit to this issue’s fork.
I did do some spelunking through commit logs, but haven't yet been able to pinpoint how and why this is suddenly triggering. I didn't get further than "this has always been here" (but a second opinion would certainly be welcome). I did not compare to when Composer actually started asking permission to run plugins, that may be a factor too.
Going down the rabbit hole where this came from. open-telemetry was originally added in 📌 Add open-telemetry/sdk and open-telemetry/exporter-otlp as dev dependencies Active , but it has no mention of this composer plugin that now has snuck in.
Update cache tags and cache contexts links in the intro (were redirecting)
Consdering the age of this, I'll close this. In general, it is unlikely to be a bug with the module if translations are not being imported.
OK, seems to still work.
Turned patch #150 into a merge request for the new module (had to do a little convincing to get the patch to apply to the standalone module). Also, updated the deprecation info (I missedn #154, but the updates in there are obsolete anyway; I take it Forum will be doing its own deprecations, independently from core).
Still need to actually tests whether it (still) resolves the problem, although I suspect the latest reports of it not working are running into the standalone module being installed, while the patch applies to the core version.
eelkeblok → made their first commit to this issue’s fork.
@Diwakar07 The other succesful lints make the build fail when they do not pass. See https://git.drupalcode.org/project/fragments/-/blob/2.x/.gitlab-ci.yml?r...
It always helps to complain (?). I think this should take care of it.
It is a quite a battle to get tests to work with the Token module. I am now running into the fact that Token module seems to be dependent on certain date formats being available.
As for the dependency, my current thinking is that maybe we should not "hard" require Token module. What that does imply is that we need to make it clear in release notes that if you need tokens derived from messages, you need to install the module.
eelkeblok → created an issue.
eelkeblok → created an issue.
An important note would be that (something like) Views Data Export is still needed when you have large amounts of data to export, because you will run into memory issues when trying to e.g. export all content of a certain type this way (depending on the mount of content in your site, of course). Another approach would be to add pagination in the JSON endpoint and have the client handle that.
One part of the tests is causing a problem, although I am puzzled by the apparent Drupal CI result that seems to suggest it is still working. It is this code fragment from Functional/WhoopsTest:
// Ensures that HttpExceptions not handled by exception subscribers
// registered before the whoops exception subscriber (e.g when the format
// is unknown) are caught and reported as whoops error page and the http
// status code is preserved.
$this->drupalGet('not-exist', ['query' => ['_format' => 'mhe']]);
$this->assertSession()->statusCodeEquals(404);
// This assertion is intentionally shorter than the full text because
// Drupal 9 and Drupal 10 have slightly different messages.
$this->assertWhoopsErrorPage('No route found for "GET');
I'll remove that for now and create a new issue to see if it should be restored. It seems reasonable behaviour that a 404 simply loads the 404 page and does not show a Whoops screen.
eelkeblok → created an issue.
eelkeblok → created an issue.
Sorry for the confusion. Something went wrong, errors are resolved after enabling the module. What remains is a functional test of the update hook.
I've just deployed the changes with the module enabled, but I still see these errors. One part I did not give much notice, but is really kind of weird, is that the error says that the theme js_cookie is not enabled...
Code looks good. I also upgraded this to major. I haven't functionally tested the changes yet.
Updated title and issue summary
Merged. Thanks!
This breaks the author summary
I added MR 20 to make sure we keep this baseline and block merges when ESLint finds stuff.
eelkeblok → made their first commit to this issue’s fork.
Wow, that was quick. I think we should also make sure GitLab breaks once we've made sure we have a baseline.
eelkeblok → created an issue.
eelkeblok → created an issue.
eelkeblok → created an issue.
Some minor changes really, plus a test to make sure this keeps on working.
Considered whether this should be a bug instead... It is a regression towards the D7 version. Then again, the "modern Drupal" version has been in existence longer than the D7 version by now.
eelkeblok → created an issue.
As a generic comment, in addition to the detailed comments in the MR, I wonder whether the buildFormFields() method could be simplified; the buildForm, which is the other place that calls this, I already checking if the field config exists, so it would be a small change to have it pass that into the buildFormFields(). If we can move the burden of getting the proper field config to the calling code (either pulling it from field storage, or from the form state when in the field creation flow), that could simplify the buildFormFields method a great deal.
Maybe this should become a follow-up, since the module is actually actively braking Drupal 10.2 sites.
Must-fix changes:
- The translated message
- Documentation for the new parameter
- Not allowing notices/warnings to bubble from the code (mostly on unset array keys; for Drupal 10.2, this happens here: https://git.drupalcode.org/project/gdpr/-/merge_requests/32#note_306162. I suspect it will happen in other places for Drupal 10.1 and below.
Depreactions are known, I believe.
I think the information that is being pulled from the field config should also be available in the form state for the form when first creating the field. We should probably make a change such that the code can get the required information from there too, if it needs to. Both for the standalone GDPR config form as well as the previous core version.
Dropping into this issue because I have a challenge with automatically created test users for each role; some of these roles have TFA, but it is a pain to actually set it up for the test users every time. So, I am looking for ways to circumvent TFA just for these test users. Would this solution still allow that, or are you saying the setting for the roles is the baseline, and other code could add more restrictions on forcing users to use TFA, not less?
This also seems to have solved the issue reported; no more gateway timeouts due to the above troublesome stack trace on pages with the voting widget.
All this currently does is update dependency information to say it works with Drupal 11, but I am not comfoitable claiming that before all deprecations are known.
eelkeblok → created an issue.
Looks good. I tried whether it is possible to have the roles as separate arguments, but it seems Drush only supports a fixed number of arguments. I did turn the password into an option and added an email domain option while at it.
eelkeblok → made their first commit to this issue’s fork.
Confirmed. Reproduced the test failure locally, but it is identical on the 8.x-1.x branch. This is with core 10.2.1. I guess the test should check for the core version it is running on.
I was trying to figure out why the test is failing, but I just realized the last time the test ran on dev was on Drupal 9.5. Since it is failing in the Field UI, which has changed significantly recently, I'm thinking this may not be due to my changes.
Yes, I agree. I like the idea of using a class a lot. We ourselves are replacing the default images with a reference to an SVG sprite, that sounds like something not too different from this.
Is this still current? If so, a warning on the project page was in order. Roller coaster of emotions here, after seeing all the good stuff on the project page 😛
eelkeblok → created an issue.
eelkeblok → created an issue.
Something like this. I considered adding static caching and methods to replace the access to the various storage handlers, but they are already statically cached within the ETM, so there would be very little advantage, at the cost of more complexity in this code.
Some "evidence", or at least some substantiation to my claims:
#3162827: Do not instantiate entity storages in constructors of services that do not always need them →
https://mglaman.dev/blog/dependency-injection-anti-patterns-drupal (see heading "Assigning entity storage to a property instead of the entity manager")
eelkeblok → created an issue.
I think I may be running into a subtle variation of this. I ran my regular maintenance script and was surprised composer installed a dev version of Upgrade Status, while my version requirement is "^4.0.0". I thought I found the reason when I say that US's composer.json mentions minimum-stability: dev, but that has been there for the past 4 years... My root composer.json has minimum stability dev, which I suppose is the reason I do not get an error like the TS, but it is installing a dev version...
This is a very puzzling addition. The link in the module page is intended for a generic settings page, that has a URL that does not require any arguments. This module has no such URL, the configured route indeed needs several arguments, so is unsuitable to add to the module overview page. Please either apply the patch of revert the commit ASAP.
Cookie-fencing modules often have a way to suppress the loading of certain Javascript. Antibot uses... Javascript to replace the action of a form. So maybe that's an area to look into.
I think this is slowly becoming a dumping ground of everything where antibot for one reason or another fails to work on the login form, which can have many different causes. We really need more than "it doesn't work" to be able to pinpoint a specific problem. *Why* was antibot unable to run its JavaScript that is supposed to replace the form action? And was that a problem with Antibot in the first place? I think we're even at a point that when someone does actually manage to put together some steps to reproduce, those would warrant their own issue (which happened in a few instances where Gin was involved), because this one is becoming too broad.
Nope.. Just tried with Gin Login on Simplytest.me, works fine. If it is related to JS aggregation, the number of modules might be a factor. Core has also seen some recent changes to JS aggregation (10.2, I think). It might be interesting to know what core versions are involved.