eelkeblok → created an issue.
I've been looking into an issue with this too, but to me it looked like the theme JavaScript was previously running before the module's JS, whereas now it is running after (I say "looked like" because I didn't go deeper into the particular issue, because it already took me long enough to get this far, and other issues doing maintenance also took plenty of time already).
I would think the latter is the correct order; first the module JS, then the theme JS. In my case, this caused problems, but in my view that means my project was (is) relying on a buggy behaviour of core. Maybe this is the case for the TS as well. However, without more details, I don't think we can determine one way or the other.
FWIW, I released a D11-compatible version of Whoops.
What would you suggest is the correct asset order? I am not quite sure what I'm looking for yet. Maybe illustrate the problem with some screenshots?
@ludo.r Can you please update the merge request? Often, it is enough to merge in the (updated) target branch. If patch no longer applies, changes are you need to solve some merge conflicts (which you probably had to do to get an updated patch).
If you have a suggestion as to the formulation of that part of the README, merge requests are welcome. Fundamentally, the WYSIWYG/RTE editor doesn't have much to do with what this module does. However, removing support for the drupal-entity tags from the input filter (and the editor configuration, when one is linked to the input filter) will mean the editor does not recognize the tags anymore, and may filter out the unknown tags; it would be critical not to save the resulting content, because that would wipe out the old tags before the module gets a chance to convert them. Also, it would be good to confirm at a HTML level (e.g. by looking in the database) if the tags were removed, or "just" not converted. You may have found another case of the module failing to recognize a certain flavour of entity embed tags.
Disclaimer: this is just what I can come up with what might have happened theoretically.
eelkeblok → made their first commit to this issue’s fork.
Heard about this only minutes ago when listening to the last Talking Drupal 😊 I have maintainer rights, but unfortunately I do not seem to have permission to add additional maintainers. Maybe I can be of help getting stuff merged, though.
@zenimagine To answer your question, I'd say it should be fine to update to 4.0, since you inadvertently were on 4 already, having installed 3.0.6. Can you set this issue to "Fixed" if your questions are answered?
@claudiu.cristea Yeah, I'm a big fan of visual Git clients, basically for reasons such as this. It becomes trivially obvious when you are always confronted with the commit graph, whereas this is a very easy mistake to make when just coming at it from the command line (not saying it is impossible to happen, of course, one can "miss-click" as well 😊).
https://www.drupal.org/project/private_message/releases/3.0.7 →
Thanks @claudiu.cristea
Part of the confusion might be 🐛 3.0.6 was released from the 4.0.x branch Active .
eelkeblok → created an issue.
Applied the review points from @penyaskito and created a new MR because the old one's target was 3.0.x.
Tests should now be green 🤞
The last hurdle was the assertion on the derived image being created. For some reason, the test couldn't assert the existence of the file, eventhough manual testing seems to work fine. Since we are basically depending on core doing its thing after we've downloaded the original, I opted to remove the check for the style image.
I restructured the branch a little because it seems some previous merge conflicts were not resolved optimally.I rebased pervious commits on that merge.
Thanks. The customer was already in the process of moving to GTM, so this was a nice extra nudge :)
It is unclear to me whether !61 is just a rebase or whether there are other differences to !38. I updated !38 by merging in 3.2.x and resolving conflicts.
Actually, there already is a method getRawText() that will do nicely.
eelkeblok → created an issue.
I updated MR !3 with a fix for the problem because I needed a solution for the time being. Leaving on Needs work for the feedback about whether we need an additional place to set these dimensions, or whether we should be looking into where the dimensions are applied when it does work and whether we can make adjustments to make that work. It seems that we are now introducing some duplicate code that could have similar bugs. I tried finding out how it works for other situations, but couldn't figure it out in the limited time I had available.
Here's a file that can be used to reproduce the problem mentioned above. It works with the default 400 wide cropping thumbnail. I didn't dive into the exact mechanics how the width ends up being larger that the available width, but it obviously is some sort of rounding error. Intuitively, I'd say a simple round should be enough to get it back to 400 instead of 400-point-something, I can't imagine the rounding error would be large enough to get it up to 401. The alternative would be to do a floor, but that would work out to 399 (as long as we're sticking to the default cropping preview for the example) in a significant number of cases.
Also, I am wondering why we need extra lines of code to pass this information to the cropper; it would seem this is already happening somewhere else, so isn't it possible to leverage that in this particular case. The solution seems a bit "smelly", but maybe I am misunderstanding the problem.
I've found this patch can actually also run into a floating point issue; on certain image sizes, the dimensions of the crop area may work out to be slightly larger than the preview image used for the cropping, in which case Cropper will decide to show the default crop area. I'll see if I can work up a reproduction case and an updated patch.
When granting credit, please also include @idebr at iO since he actually found this issue.
That last commit was me. I did it through the web interface, so that's probably the reason it doesn't properly show up.
From my limited testing, I noticed that only when the field is empty, the value will indeed be an array. I think the solution from the previous patch is than in fact not quite appropriate; it just takes one of the empty values and passes it through the test of the code. Instead, I changed it to treat it similarly to a NULL value; just bail out when it is an array.
Still needs a test, of course :)
eelkeblok → made their first commit to this issue’s fork.
It works, but I am a little worried about how future proof this solution is, replacing the core version of the file with its own. I guess it will at least need a test to make sure it continues to work (note that testing are currently not working, there is a ticket to get test running working again on GitLab, see 📌 Fix coding standard issues + add GitLab CI to the project Needs review ). I also had a look at gin layout builder that you mentioned, but it is far more elaborate, taking also into account stable theme and what not.
I had a quick look if it would make sense to amend the core version with an additional file, but due to the nature of the reset, that wouldn't work.
Linking 📌 Fix coding standard issues + add GitLab CI to the project Needs review as it fixes the test that I guess needs to be extended to fulfil #15.
I scanned through the changes and didn't seen anything major. If there are any left-over concerns, I would advocate for getting the Gitlab and test-changes in first, and then creating follow-up issues for the various linters. I just spent about half an hour hunting down tests breaking in the module because I wanted to try to get 🐛 Crop widget not adjusting crop box when crop is applied Needs work over the last hump by writing a test. I first found out tests haven't been running since there is no Gitlab integration and then I found this issue...
I agree with #5. I think it would be a good idea to add some verification to make sure that the crop preview does not include any crops.
I would suggest to make this configurable, at least, and make sure that the behaviour remains the same for existing installs (what the default should become for new instals would be up for debate). But maybe you first want to hear from the maintainers how they feel about this suggestion.
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.