Netherlands 🇳🇱
Account created on 28 January 2009, over 16 years ago
  • Lead Drupal Developer at iO 
#

Merge Requests

More

Recent comments

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Updated the issue summary.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Would you please assign credit? It is important to my company. Thanks!

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Great idea.

To answer #15, I guess the generic use case is when files are generated on the system instead of uploaded. In our particular case, we are generating PDFs from Drupal content, using node titles (and other field values) to determine the file name. These may include characters that are not suitable for filenames. We are phasing out the filename_transliteration module, only to find that it is not possible to call the core logic like it is with that module.

Updated IS.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Weird. I looked through my Git history, but no details. Only confirmation that it was for Platform.sh. I do see that the project is on Redis 6.0 and has been since we added Redis. That's not supported anymore, not sure what the status was when it was added.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

FWIW, I can't reproduce the original problem anymore. 🙃 I had this patch for a site on Platform.sh, maybe they've changed something.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

I'm a little doubtful, as 📌 Abstract info-collection RedisController to deal with special environments Active is still calling config() to retrieve the value. I have been running with #28 on a project for a while now, that eliminates that completely. But maybe the condition in there is enough to prevent that from happening.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Refactored to use the new OO hook pattern.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Sorry to bump a closed issue, but I think a new issue might be overkill. I just spent some time debugging why Linkit was no longer working on my site. Turns out, this issue is "to blame", because my input formats did not have the filter enabled; we were in fact relying on the filter being optional. Might it be an idea to add a snippet to the releases notes for 7.0.7 stating as much? E.g. "Note: Any input filter where Linkit should work now must have the Linkit input filter enabled."

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

The project is on Drush 13. I did find mention of a global -u option to set the user ID and the batch processing in Drush is actually trying to set it somewhere, but apparently this was removed in Drush 9, so that can hardly be a factor. Something is still off, though, since tests are failing.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

The patch in 3150 unfortunately resolves the merge conflict between this patch and D10.5 incorrectly. The conflict is that the FileWidget settingform introduces a warning if the upload progress PHP extension is not available. The patch in #150 removes that again.

Here is a patch for D10.5 that retains the notice.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

I also created MR 16 that simply adds the missing load include. Feel free to use whichever you like.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

I think this is now good to go. Maybe a bit ambitious for a simple bug fix, let me know what you think.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Very sorry to re-open an old issue, but I have been running with the patch for this issue for quite some time. I just realised I still had this patch while the current release is marked as compatible with D10, so I wondered whether there was something else that fixed the problem. Removing the patch got me right back to this issue. I reproduced on Simplytest.me to rule out it was anything in my setup, and it is reproducible with a plain D11.2 as well. I tried to clarify the steps to reproduce.

If you would prefer to open a new issue, I would be happy to do that, but because it seems the original issue is is simply not resolved, I elected to re-open this one.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Examining the code, it seems to be a simple matter of a missing moduleHandler->loadInclude() in that hook. Maybe, though, this is a good opportunity to start converting Drupal 7-style includes to a modern Drupal serviche architecture.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

The entire article looks quite outdated, because:

  • It is leaving open not using https as an option, and is talking about mixed mode, which nowadays is basically just a hassle and not worth it
  • It is talking about Drupal 7

I reworded the section "Why is it important to you (and when)" to take this into account, but I ran out of time, so the rest of the article needs to be done. I think it can basically be simplified quite extensively, by removing all explanations about how to get stuff to work in mixed mode.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Looks good. The test verifies the content of the displayed diff. I triggered the test-only pipeline, but that's silliness, of course, since we're not solving a bug. I functionally verified the patch in our project, code changes are minimal.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Since I am having difficulty getting the issue reproduced (and the bug report in our project is a little old), submitting a branch with only the test changes to see if it is still broken.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

I just submitted the most basic MR implementing this idea, because I was running into the exact scenario of the duplicate search form. I did include the test patch, but nothing in the way of deprecation code yet. I did ponder it for a bit, but it is not evident to me how to deprecate this properly. Do we include a whole new function that people should call instead? If so, should this issue cover all core use of the old function?

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

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

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Correct me if I'm wrong, but shouldn't Simple XML Sitemap keep track of dependencies to other modules when a plugin is added to its configuration?

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

🙌 Excellent. I'll see if I can take a crack at that soon.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

I ran into what appears to be the same issue and I think it is mostly unrelated to migration. There is an easy workaround, which is to remove the plugin reference from the configuration. Nevertheless, it can give some nasty effects when disabling a module that contains UrlGenerator plugins that are in use. I added some steps to reproduce.

I *assume* the OP was having this situation too. If not, my apologies. Let me know, than I'll create a different issue. Of course, the same could happen if the "custom" plugin got its plugin ID changed, which is basically the same thing; the configured plugin is no longer available.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

I just found 📌 Support OOP hooks Active and noticed there that currently most hooks are implemented using the OO class_resolver pattern, which makes the official OO hooks conversion much easier. So we should at least change that, possibly do it all in one go if 📌 Support OOP hooks Active lands first.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Ah, I had not noticed this issue before. I added a hook_cron implementation in Add a cleanup function Active and did not use this pattern because of the Drupal 9 support (although I may have used some PHP features that rule out D9, not sure🫢). Will probably depend on the sequence of merging what needs to be adapted to what.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

As expected, this didn't go flawlessly, I ran into out of memory exceptions.

  • I added a batching operation, deleting 100 log entries at a time, with an cache clear for the entity storage in between.
  • I added a time limit, so the hook cron will not run longer than 10 seconds.

Typically, I would say this is enough for most setups. Maybe it is worth mentioning somewhere in the documentation that if you consistently get warnings that the time limit was exceeded, you need to either increase the frequency the cron hook is ran (maybe with a reference to ultimate cron) or reduce the amount of log entries created.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Added a quick implementation using Cursor. I am not convinced the "just delete everything older than x" through the entity interface is a great idea. Especially if there is a large backlog I don't think it will finish in a reasonable amount of time. But maybe I'm mistaken, needs a little more testing (like with a prod database from my example project :) ).

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

See https://www.drupal.org/node/3082634 . What this issue seems to need is:

  • An updated change record
  • A call to Drupal.deprecationError() referring to said change record
  • A follow-up issue to actually delete the functionality in Drupal 12

Updated issue summary.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Adding the issue that this MR is currently rolling back.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

@steven jones Thanks for that clarification. Our use case does not rely on the file being managed, so your proposal sounds fine.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Strictly speaking, when this is a security vulnerability, this should have been reported through https://security.drupal.org. The impact is fairly small, although depending on site styling an "empty" section might still have some impact on a page, I suppose. Maybe best to do so still, just to be on the safe side. Maybe the security team has means to remove public reference to the issue (like also the Git commits).

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Our use case is pretty simple: the file needs to be available at a specified location, under a specific, descriptive name. We are actually generating the file in the private file location, although I could imagine the need to put it somewhere else. I'm not sure what you mean by "I wonder about leaving the generation of the file alone, and then at the end of the Drush command, it can simply copy the file to the new location." Do you mean to keep the different name out of the scope of this Drush command and leave it to the caller to move the file afterwards? In our case, that would make things more complicated, as we currently have a single line in our crontab to generate the file in a specified location using this command. I suppose it is possible to do that all in a single line in a crontab, but I'd also say that allowing the user to specify a file name for the export file is a very reasonable feature to have.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

#34 (i.e. https://www.drupal.org/files/issues/2020-08-05/2887450-34_0.patch ) is a very old patch... Are your patches based on the patch or the merge request?

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

I've hidden MR 5 en 48. 5 was superseded by 14, as mentioned in this MR comment. 48 seems to be a completely different approach to the issue and doesn't do what the issue title says ("do not reimport existing entities"). Judging by the title, it first removes existing entities and then reimports the entities from default content, which is definitely a legitimate scenario, but something else entirely. I think it probably warrants its own issue (which should probably be postponed on this one, to keep things manageable).

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

eelkeblok changed the visibility of the branch 2698425-delete-existing-n-import to hidden.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

eelkeblok changed the visibility of the branch 2.0.x to hidden.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

eelkeblok created an issue.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Looks like this was resolved in a different way in the latest release ( 3.0.3 ).

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

@claudiu.cristea would you maybe assign some credit for this issue? It is valuable for me and my company. Thanks in advance.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

eelkeblok changed the visibility of the branch 2928564-add-stagefileproxy-stream to active.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

eelkeblok changed the visibility of the branch 2928564-add-stagefileproxy-stream to hidden.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

WRT the origin_path setting, there seems to be some doubt as to whether support for private files belongs in this issue. I would say, no. There already is a lot of value in converting only public files to using stream wrappers (e.g. the use case with responsive images, where the responsive image module tries to open the physical file to get information on its size), and adding private file support into the mix complicates this issue. I think my next task will be removing support for private files from the merge request. Maybe we should consider re-opeing #2879046: Compability with private files and postponing it on this?

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Going to call this, a bit opportunistically, a bug, because that would mean I can still include it in the 2.1.x branch.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Forgot to actually merge.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

eelkeblok created an issue.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

The question is, would it be possible to device something to support the batch that is potentially built by the hook? If so, that would also open up an opportunity to remove the code that reimplements the core cancellation methods.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Ah, no, it was meant to say I adjusted the MR. Fair enough to get some tests.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Can you elaborate on the reason this needs work? AFAICT the merge request is mergeable.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Thanks. The latest commit fails its tests, but that is not due to these changes.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Thanks. The latest commit fails its tests, but that is not due to these changes.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

It seems the Gitlab job for the next major Drupal version (i.e. 11) is actually failing. I did not dive in too deep to see what is going wrong, but it seems a pretty fundamental problem for this particular issue :)

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

I put a reference to the Backdrop version on the project page. I'll close this issue as fixed.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

This project is no longer maintained. Closing.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Drupal 7 has reached end of life. Closing.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

This project is no longer maintained. Closing.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Drupal 7 has reached end of life. Closing.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Drupal 7 has reached end of life. Closing.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Drupal 7 has reached end of life. Closing.

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Drupal 7 has reached end of life. Closing.

Production build 0.71.5 2024