Drupal 10 compatibility fixes

Let's merge this one in order to keep code updated in dev branch
once jquery_ui solved - could be tagged in a release

I think this needs an update hook to enable the jquery_ui_draggable module

One question.. If this is dependent on the jquery_ui_draggable module, shouldn't it be jquery_ui_draggable:jquery_ui_draggable instead of drupal:jquery_ui_draggable in the info,yml?

https://git.drupalcode.org/project/focal_point/-/merge_requests/3/diffs#...

Hi,
I have added a diff.txt in comparison with MR!3.
Added the contributed jQuery UI dependencies in info.yml and composer.json

Please undo the composer changes. They are not needed, In fact, deleting drupal/crop from the composer dependencies would improve the module.

Looks ok

Tests are failing?

It looks like the composer requirements are needed for tests. We can add them as require --dev .

Please undo the composer changes. They are not needed, In fact, deleting drupal/crop from the composer dependencies would improve the module.

This module's info.yml file list crop as a dependency. So either crop is a dependency and can stay in the composer.json, or is not and so it must be removed from .info.yml.

In addition to that, I believe the composer.json file has now no use and can be removed?

We can do this work in a separated issue, but my understanding is if you now require the new version of the module installing it will fail.

The composer json requirement will be added by drupal.org. It's the reason why the module worked fine before and still would work fiine.

AFAIK if a composer.json file exists in the repo, drupal.org won't touch it.
So far - branch 8.x-1.x - focal_point's composer.json requires drupal/crop.

BUT

I tried locally and it works, dependencies are fetched. So why are the tests failing with:

Unable to install modules: module 'focal_point' is missing its dependency module crop.

Indeed, I was reading about this since I wrote #8, and for the case of MR tests (not patch file tests), the testbot is unable to find the dependencies. It's just for the case of testing, and you if you try to composer require the latest 2.x-dev, you will see that drupal.org will complete the requires also for modules that have a composer.json.

But you're right. I was wrong in #8 to say they are not needed. MR tests will fail if those lines are not there. Honestly, seems like a bug in the MR tests, as the patch tests will work fine, and so will normal module installation usage.

Right this seems a bit weird, should we add the crop module requirement back so that we can get this merged?

Yes, but the requirements need to be:

    "drupal/crop": "^2.3",
    "drupal/jquery_ui": "*",
    "drupal/jquery_ui_draggable": "*"

For now at least. There's still no version of the jquery modules supporting Drupal 10, and until then, we don't really know if they'll jump to version 2 for Drupal support or not. Once those modules launch a D10 release, the dependencies can be fixed at the correct versions. For crop, it needs to be 2.3 as that was the first to support Drupal 10.

I do wonder however, why the module is requiring for D10 support a module that doesn't support D10.

> I do wonder however, why the module is requiring for D10 support a module that doesn't support D10.

because those jquery ui dependencies are being removed from core.

But yeah, we need #3277748: Drupal 10 compatibility to land. Once that's in, it will be either simple to update all the other jquery modules or we can then just drop them in case jquery_ui decides to inline everything.

@berdir yes thats correct.

But yeah, we need #3277748: Drupal 10 compatibility to land.

JQuery UI is now Drupal 10 compatible!

And so is #3288104: Automated Drupal 10 compatibility fixes, so all the new dependencies are ready for D10. Time to set the new constraints to:

    "drupal/crop": "^2.3",
    "drupal/jquery_ui": "^1.5",
    "drupal/jquery_ui_draggable": "^1.5"

Working during porting day. Tagging for visibility.

Based on #24, looks like someone can create a patch/MR to handle these changes.

Hi everyone,
The missed dependencies have been added, please review.

I am unable to drag the focal point crosshair to select the focal point, also getting some errors in browser console. Screenshot attached as below imgur https://imgur.com/a/soa1zGh

Hi everyone,

@tdnshah thank you for reviewing.
I've update MR to fix JS errors. Please review once again.

Thanks for the update! I see some errors in the tests:

https://www.drupal.org/pift-ci-job/2526051

so moving back to needs work.

Seems like for some reason as @jcnventura in #18 have already said, testbot is unable to find the dependencies for MR changes, which is weird. However, there are actual tests that are failing for 2.x-dev which can be seen in this CI job: https://www.drupal.org/pift-ci-job/2511924

The latest commit updating the parameters' types is fixing this problem, and with the changes everything is still working correctly in D10, and D9.5. I'm against something to be merged if the tests are failing, however since it is still a development branch and we can tag a release any time, I would say in this particular case we could try to merge the changes and see if the tests will pass for the actual development branch once someone else has reviewed and tested it manually.

(If there are tests still failing after the merge we can open a follow-up issue to fix those failing tests if there are any.)

Also, there is a related issue in RTBC for the jQuery.once changes which could be closed once the changes for this MR is in (and possibly the credit for that issue moved here, but I'd leave that to a maintainer to decide):
https://www.drupal.org/project/focal_point/issues/3324563

The only small problem I found during manual testing is the placement of the Preview link for the images, which is overlapping the thumbnail for the image field. It is not actually blocking D10 Readiness, and there is an open issue for that: https://www.drupal.org/project/focal_point/issues/3265251

3265251 - #3265251: Preview link overlaps with image on Claro theme is fixed in 2.x :)

Based on #36, rebased the branch to have the changes from #3265251: Preview link overlaps with image on Claro theme

MR !8 is merged in 2.x branch as per #33 and keeping this open.

Seems like the tests are passing after the merge: https://www.drupal.org/node/2144115/qa

2.x-dev test with PHP 7.4 & MySQL 5.7, Drupal 9.4.x
2.x-dev test with PHP 8.1 & MySQL 5.7, Drupal 9.5.x
2.x-dev test with PHP 8.1 & MySQL 5.7, Drupal 10.0.x

I'd say we can close this issue and open up a new one if there is a bug/issue with the changes.

With Drupal 10 released, we can close this as the update bot won't be delivering more changes.

A bit confused about jquery_ui_draggable, it has a 2.x release now but the 1.x release is apparently also D10 compatible? This module requires 1.x, would be good to allow 2.x as well I suppose.

#41: This is not a project update bot issue, it wouldn't deliver any changes anyway.
#42: That's a nice catch, with manual testing the 1.x version worked just fine, but 2.x requires a newer jQuery UI version, I'd leave this to the maintainer to decide, but I don't see why we wouldn't update to 2.x

As per #38, this issue seems to now be fixed. I agree with #42 and #43, but at this point, maybe that should be a new issue?

created separate issue for upgrade of Jquery UI Draggable.
#3327967: Update jQuery UI Draggable to 2.x

Are there any updates on this? Was there a patch that eventually ended up working? Edit: Oh I see this release - composer require 'drupal/focal_point:^2.0'