Problem/Motivation

Let's make FileField Sources Drupal 10 ready!

Remaining tasks

CommentFileSizeAuthor
#47 2x-d10-ready.patch15.57 KBrschwab
#35 3336268-6-filefield_sources-drupal-10-compatibility.patch41.78 KBdanheisel
#21 filefield_sources_d10compat_3336268.patch24.56 KBrobcarr
#15 issue-3336268-installed-ok.png12.7 KBoptasy
#13 3336268-3-filefield_sources-drupal-10-compatibility.patch8.82 KBdan.d
#7 single-export-filefield_sources-2023-02-15T11 32 16+0000.txt35.42 KBalexp999
#2 3336268-2-filefield_sources-drupal-10-compatibility.patch1.75 KBjrglasgow

Issue fork filefield_sources-3336268

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jrglasgow created an issue. See original summary.

jrglasgow’s picture

jrglasgow commented
StatusFileSize
new 3336268-2-filefield_sources-drupal-10-compatibility.patch1.75 KB

updated filefield_sources.info.yml and composer.json to include D10 compatibility, including the patch for #3325652: deprecated alter hook hook_field_widget_form_alter()

jrglasgow opened merge request !5

nickspages’s picture

nickspages commented

Tried this patch with my Drupal 10.0.2 and it did not work.

loopy1492’s picture

loopy1492 commented

I applied the patch for MR5 to 1x-dev and the Upgrade Status module seems happy with the changes on D9.5.3 with php 8.1.

loopy1492’s picture

loopy1492 commented

@nickspages did the patch just not apply for you or did you receive some other kind of error?

alexp999’s picture

alexp999 commented
StatusFileSize
new single-export-filefield_sources-2023-02-15T11 32 16+0000.txt35.42 KB

I'm still seeing 49 errors and 2 warnings with the patch in #2 using upgrade status.

Lots of calls to deprecated functions.

jrglasgow’s picture

jrglasgow commented

I finally have a D10 environment set up to start testing, I will be making some more changes this week.

jrglasgow’s picture

jrglasgow commented

this is going to require a major version change as it is not backwards compatible with Drupal 9 or Symfony 4

jrglasgow’s picture

jrglasgow commented

after looking at this a little bit more I see that the Symfony 4 version of symfony/http-foundation which contained the MimeTypeGuesserInterface which filefield_sources uses is there, but also symfony/mime ^5.3 which contains the same interface (different namespace) is including in drupal/core since 9.2.0, so this module CAN be compatible with ^9.2 | ^10, so we SHOULDN'T necessarily need a major release.

i-trokhanenko’s picture

i-trokhanenko
Location Lutsk 🇺🇦
commented
Status: Active » Needs review
dan.d’s picture

dan.d commented

This patch seems to be incomplete. There is another issue that needs to be addressed to make this module compatible with D10:

Call to deprecated function file_munge_filename(). Deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Dispatch a \Drupal\Core\File\Event\FileUploadSanitizeNameEvent event instead.

L475 - https://git.drupalcode.org/project/filefield_sources/-/merge_requests/5/...
L596 - https://git.drupalcode.org/project/filefield_sources/-/merge_requests/5/...

dan.d’s picture

dan.d commented
StatusFileSize
new 3336268-3-filefield_sources-drupal-10-compatibility.patch8.82 KB

I'm posting the updated version of the patch, which seems to be completing the list of deprecations for D10. Any feedback will be appreciated.

aitala’s picture

aitala commented

Testing this on D 9.5.11 and it seems to work nicely.

Thanks,
Eric

optasy’s picture

optasy commented
StatusFileSize
new issue-3336268-installed-ok.png12.7 KB

Tested this on a D10 instance, works ok, README.txt file needs to be renamed to README.md.

webengr’s picture

webengr commented

out of time, 2 weeks till end of life for drupal 9, this is obsolete.

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented
Status: Needs review » Reviewed & tested by the community

Just upgraded a couple of sites to D10 and this patch seems to work fine. Thanks for your work.

Needs a bit work to improve on coding standards then test will pass

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented
Status: Reviewed & tested by the community » Needs work

Sorry, this module needs some basic work to pass tests. Mostly code formatting...

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented

Running the module through PHPCS it's riddled with (mostly trivial) coding standards errors. I'll try and do what I can do work on these, but I struggle with tests, so might need support

Other issue I've noticed is that current formal release (8.x-1.0-alpha5) was released 19 January 2022, whereas Dev release is far older (over 2 years since last commit: 24 May 2021). So I'm going to base all my updates on the alpha5 branch

chetan 11’s picture

chetan 11 commented

I'm working on the phpcs issues.

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented
Version: 8.x-1.x-dev » 8.x-1.0-alpha5
StatusFileSize
new filefield_sources_d10compat_3336268.patch24.56 KB

Patch to look at D10 compatibility and address all coding standards issues. Would welcome a review and maybe some additional work will be required on tests.
Have rolled patch agains 1.0-alpha5 as it's much more current then DEV release.. although I suppose that might fail automated tests in itself

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented

Previous patch does not apply :(

natkeeran’s picture

natkeeran commented

Applied path #13 patch to 1.0-alpha5 ( D10.1.5, php 8.1) . Throws the following error.

Error: Call to undefined method Drupal\Core\ProxyClass\File\MimeType\MimeTypeGuesser::guess() in filefield_sources_save_file() (line 449 of /var/www/D9-collections3/web/modules/contrib/filefield_sources/filefield_sources.module).

Looks like guess method does not exist in D10, and need to use guessMimeType.

bobi-mel’s picture

bobi-mel commented
Assigned: Unassigned » bobi-mel
bobi-mel’s picture

bobi-mel commented
Assigned: bobi-mel » Unassigned
Status: Needs work » Needs review

@Natkeeran I fixed. Please check

jamesdriscoll’s picture

jamesdriscoll commented

8.x-1.0-alpha5 is reporting 55 problems.
I'm happy to test but what's the sequence of patches that need to be applied to get to a 'now fixed' state? (it's clearly more than the mimetype fix)

rschwab’s picture

rschwab commented
Version: 8.x-1.0-alpha5 » 8.x-1.x-dev
Status: Needs review » Reviewed & tested by the community

I just tested this branch out on a fresh 10.1.7 install and it works just as expected. I tested image, text, and tgz files using upload, remote, reference existing, and attach. The only thing I saw were php warnings that are already noted in #3313074: Warnings while adding a remote file after switching to PHP 8.0.

+1 for RTBC

rschwab’s picture

rschwab commented
Status: Reviewed & tested by the community » Needs work

More work is needed to make the test suite happy.

rschwab’s picture

rschwab commented

For speed and to keep it simple, I'm merging the work from #3168858: drupalPostForm() in functional tests is deprecated and #3408263: Replace deprecated SimpleTest assertions here.

rschwab’s picture

rschwab commented
Issue summary: View changes
rschwab’s picture

rschwab commented

As of now all tests are passing, except for 4 failures that would be solved by merging #2840594: Dont use curl, use the drupal client instead . Once that is resolved this should be ready as well.

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

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

uridrupal’s picture

uridrupal commented

The last changes broke the code, there's an extra } that has been deleted

danheisel’s picture

danheisel commented
StatusFileSize
new 3336268-6-filefield_sources-drupal-10-compatibility.patch41.78 KB

Hoping this patch sorts out the missing curlies from the MR above. I saw two in Remote.php. Seems to be running fine now on my sites.

uridrupal’s picture

uridrupal commented

Seems patch #35 works fine for me. I have learnt the risk of using a Merge diff as a patch now.
Thanks!

rschwab’s picture

rschwab commented

I think code standards fixes should go into their own issue so as not to confuse the issue at hand. The commits in #32 and the patch in #35 seem to be mistakenly removing needed code in Remote.php.

szato’s picture

szato commented

Confirmed: patch #35 works, MR5 has syntax error:
PHP Parse error: Unclosed '{' on line 345 in /var/www/html/web/modules/contrib/filefield_sources/src/Plugin/FilefieldSource/Remote.php on line 351

But there is a new 2.0.x branch.

mlncn’s picture

mlncn commented
Status: Needs work » Needs review

Made a 2.0.x branch based on all the work here and danheisel's fix. (Gnuget gave me maintainership a week or so ago; if anyone else has put themselves forward let me know!)

If no fatal errors found i think we can finally mark this fixed and keep anything else in follow-up issues.

mlncn’s picture

mlncn commented
Version: 8.x-1.x-dev » 2.0.x-dev
rschwab’s picture

rschwab commented
Status: Needs review » Needs work

The code in #32 and the patch in #35 both include removal of code from src/Plugin/FileFieldSource/Remote.php that should not be removed. Changing this to Needs Work. My (probably biased) opinion is to take the code up to #29 and have the two folks making code standards changes in #32 do that work in a separate ticket.

szato’s picture

szato commented

1) diff between MR5 and 2.0.x branch in only the missing closing "}" - fix made in#35:
https://git.drupalcode.org/project/filefield_sources/-/compare/3336268-d...

2) @rschwab, if I'm correct, you are talking about the removed code (from file: Remote.php) in this commit:
https://git.drupalcode.org/project/filefield_sources/-/merge_requests/5/...

3) @jrglasgow can you please (as an author of the MR) edit the MR and change the target branch to 2.0.x?

rschwab’s picture

rschwab commented
Issue summary: View changes
rschwab’s picture

rschwab commented

To try and make this as easy as possible, I've opened #3416606: Restore functions to remote.php as a child of this issue. Once that and #2840594: Dont use curl, use the drupal client instead are reviewed and merged I believe this issue will be fully resolved.

rschwab’s picture

rschwab commented
Issue summary: View changes
rschwab’s picture

rschwab commented
Issue summary: View changes
rschwab’s picture

rschwab commented
StatusFileSize
new 2x-d10-ready.patch15.57 KB

Here's a patch for anyone who would like to get this module working in D10. It incorporates the work as of today in the following issues:

It should apply cleanly against the 2.0.x-dev branch as of today.

sseto’s picture

sseto commented

Will we get a stable D10 release back soon or should I switch to the dev version and apply the patch from #47?

Thanks!

edboost’s picture

edboost commented

@Sseto: Did you end up trying this? Trying the module with the #47 patch? How did it go?

sseto’s picture

sseto commented

I swapped to dev version and so far so good :)

jrglasgow’s picture

jrglasgow commented
Title: Drupal 10 compatibility » Drupal 10/11 compatibility
Issue tags: +Drupal 11 compatibility

I updated the fork to be compatible with Drupal 11 (at least removed Drupal 11 deprecations) I haven't yet tested.

PHP Stan still finds a few more issues of deprecations that were just deprecated in Drupal 10.3 (a few weeks ago at this posting) and aren't being removed until Drupal 12. I didn't want to exclude anyone who isn't on Drupal 10.3 yet.

 ------ ------------------------------------------------------------------------------------------------------
  Line   filefield_sources.module
 ------ ------------------------------------------------------------------------------------------------------
  439    Fetching deprecated class constant EXISTS_RENAME of interface Drupal\Core\File\FileSystemInterface:
         in drupal:10.3.0 and is removed from drupal:12.0.0. Use
         \Drupal\Core\File\FileExists::Rename instead.
  446    Fetching deprecated class constant EXISTS_RENAME of interface Drupal\Core\File\FileSystemInterface:
         in drupal:10.3.0 and is removed from drupal:12.0.0. Use
         \Drupal\Core\File\FileExists::Rename instead.
  521    Fetching deprecated class constant EXISTS_REPLACE of interface Drupal\Core\File\FileSystemInterface:
         in drupal:10.3.0 and is removed from drupal:12.0.0. Use
         \Drupal\Core\File\FileExists::Replace instead.
 ------ ------------------------------------------------------------------------------------------------------
jerech’s picture

jerech commented

This patch is giving me a mistake: patch -p1 < filefield_sources_d10compat_3336268.patch

alex.mazaltov’s picture

alex.mazaltov
Primary language Ukrainian
commented

Looking to the fork implementation for d11 compatibility.

I am wondering if we can generate a patch in an old-school way to test the implementation with alternative commerce Drupal core to make it compatible with d11

c-logemann’s picture

c-logemann
he/him
Primary language German
Location Frankfurt/M, Germany
commented

@alex.mazaltov Do you mean the diff or patch export option of the merge request?
It's at the dropdown menu of the "code" Button on each MR.
For the MR mentioned above it's this:
https://git.drupalcode.org/project/filefield_sources/-/merge_requests/5....
https://git.drupalcode.org/project/filefield_sources/-/merge_requests/5....

Example of how to use with Composer patches and lenient:
https://www.drupal.org/project/boost/issues/3428282#comment-15948406

carlos romero made their first commit to this issue’s fork.

mlncn’s picture

mlncn commented
Status: Needs work » Reviewed & tested by the community

Brought all the updates in:

https://git.drupalcode.org/project/filefield_sources/-/commit/642a01f04e...