🇺🇸United States @m.stenta

I opened a PR that changes how we maintain CHANGELOG.md moving forward: https://github.com/farmOS/farmOS/pull/948

I also started a draft PR for starting an official 4.x branch: https://github.com/farmOS/farmOS/pull/949

Once we merge that, then we can push it to https://git.drupalcode.org/project/farm and create a 4.x-dev snapshot, which will allow us to start referencing that version in this issue queue.

Discussed this on the farmOS dev call today. We will probably just make the changes to the farmOS (and Log module) plugins in the 4.x-attributes branch, rather than trying to get it into 3.x.

There is a draft PR for a Rector rule that automates this conversion here: https://github.com/palantirnet/drupal-rector/pull/308

I'll reverse that commit and see if it makes any difference. First look is that it's only the category name that has changed. Hard pressed to think that would make a difference.

That was my thought too, but maybe the category gets used somewhere along the way. Just a guess...

@elc What version of Drupal core and Fraction are you using?

I wonder if this change has anything to do with it?

https://git.drupalcode.org/project/fraction/-/commit/f34523b9fe859f80745...

That is the only substantive change to the 2.x branch since January.

Key to it is that $field->field_alias == 'unknown' for all of the Fraction fields in a view. That was causing the complete failure of what this patch was doing.

Otherwise, I wonder if something changed in Drupal core / Views that could explain the new behavior you're seeing. 🤔

I have tested and updated the merge requests for both of the contrib module's we're waiting for:

• entity_reference_validators: 📌 Automated Drupal 11 compatibility fixes for entity_reference_validators Needs review
• jsonapi_schema: 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review

With both of these in place, I was able to successfully install farmOS 4.x on Drupal 11 in my local dev environment! 🎉

There were a bunch of new PHPStan errors, so I fixed those as well. PHP CodeSniffer and Rector are both clean. I have not run automated tests yet (it will be easiest to do that via GitHub Actions).

I will rebase and push all my 4.x-* branches to my GitHub fork once we release 3.4.4. Then we can consider if we're ready to start an official 4.x branch and start opening PRs against it.

Pushed a commit that fixes the /api/[entity-type]/[bundle]/resource/schema routes.

This Drupal core change record describes the change that was necessary: https://www.drupal.org/node/3359695 →

Back to Needs Review!

The root /api/schema endpoint and collection endpoints (/api/[entity-type]/[bundle]/collection/schema) seem to be working.

Not working yet...

I tried to view a schema endpoint (/api/[entity-type]/[bundle]/resource/schema) in my browser and the following error occurs:

Uncaught PHP Exception Symfony\\Component\\Serializer\\Exception\\NotNormalizableValueException: "Could not normalize object of type "Drupal\\Core\\Field\\BaseFieldDefinition", no supporting normalizer found." at /opt/drupal/vendor/symfony/serializer/Serializer.php line 181

Opened a merge request with @volha_si's commit from 🐛 PHP8.3 compatibility issues Active (I updated the commit message to be more descriptive of its purpose, but kept the author credit).

I also added a commit that drops support for Drupal 8, 9, and 10. So this will need to be a new major release (not on the 8.x-1.x branch) - pending @bradjones1's response to my question above (#10).

I will do more testing locally and report back if I find any other issues.

Thanks @volha_si!

Quoting myself from 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review :

This error is actually a result of changes in the upstream Symfony class that we extend from. So I don't think it's specific to PHP 8.3.

We can probably close 🐛 PHP8.3 compatibility issues Active as a duplicate of this issue, and incorporate the MR changes into here. I'll start a new MR and cherry-pick them over.

I'm going to close this as a duplicate and see if I can incorporate your changes into 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review .

@m.stena Probably related to this: https://www.drupal.org/project/jsonapi_schema/issues/3504440 🐛 PHP8.3 compatibility issues Active

Thanks @alexander-tallqvist!

This error is actually a result of changes in the upstream Symfony class that we extend from. So I don't think it's specific to PHP 8.3.

We can probably close 🐛 PHP8.3 compatibility issues Active as a duplicate of this issue, and incorporate the MR changes into here. I'll start a new MR and cherry-pick them over.

Fatal error: Declaration of Drupal\jsonapi_schema\Normalizer\DataDefinitionNormalizer::normalize($entity, $format = null, array $context = []) must be compatible with Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize(mixed $data, ?string $format = null, array $context = [])

I think fixing this is going to necessitate dropping support for Drupal 10.

@bradjones1: I see that you have started a 2.x (and 2.0.x) branch that makes some of these changes already. Should I base a MR off of one of those? I can't find any mention of them in the issue queue, and there are a number of other changes. Any guidance you can provide on your intentions would be appreciated! I want to help push this forward, but don't want to step on toes or duplicate efforts.

I tested installing on Drupal 11.1.5 and it failed with the following error:

$ drush en jsonapi_schema

Fatal error: Declaration of Drupal\jsonapi_schema\Normalizer\DataDefinitionNormalizer::normalize($entity, $format = null, array $context = []) must be compatible with Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize(mixed $data, ?string $format = null, array $context = []): ArrayObject|array|string|int|float|bool|null in /opt/repos/jsonapi_schema/src/Normalizer/DataDefinitionNormalizer.php on line 53
 [warning] Drush command terminated abnormally.

I had to fix this in farmOS as well, so I'm familiar with the solution. I'll work on this...

Thanks for testing @onfire84 and @alexander-tallqvist.

Let's get confirmation that this works on Drupal 11. Setting back to Needs Review.

Rebased onto 11.x.

The biggest change was to move the node and taxonomy hook implementation changes to Hook classes, since 📌 [ignore] Convert everything everywhere all at once Active was merged.

Also attached (and hidden) is a patch that works with 11.1.x, since that has some slight differences, and is useful for applying to the current stable release (11.1.5) via cweagans/composer-patches.

I wonder if we should consider deprecating the Asset/Log/Plan/etc events, and just recommend using the core hooks as intended.

I opened this related issue, which IMO we should tackle first: 📌 Convert all procedural hook implementations to Hook classes Active

Postponing this issue, but let's review it in more detail to see if there's anything we can work on or plan for in the meantime.

We have another issue open that overlaps/relates to this: 📌 Replace farmOS hooks with Events + EventSubscribers Active

I think we can work on this issue as the first priority, since we should follow core's pattern first and foremost.

IIRC the other issue proposes some other changes, so let's review that again and see if/how it needs to be updated

m.stenta → created an issue.

I would argue this is a case where a test isn't needed, and may not be possible... but leave that to others to decide.

there is policy for small changes not needing test coverage https://www.drupal.org/about/core/policies/core-change-policies/core-gat... → . Just a matter of proving the points are hit to not need them.

Copied and answered the questions below (to the best of my understanding):

An automated test for a bug fix may not be needed if the following are true and if the majority of answers to the questions below is 'Yes'.

1. The issue has clear ‘Steps to reproduce’ in the Issue Summary. ✔️
2. The fix is 'trivial' with small, easy to understand changes. ✔️

Questions

1. Is the fix is easy to verify by manual testing? YES
2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc. YES
3. Is the fix achieved without adding new, untested, code paths? YES
4. Is an explicit 'regression' test needed? NO
5. Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue? NO
6. If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed? NO

I removed the "Needs Tests" tag, but please restore it if you disagree.

Tests pass!

There was one minor PHP CodeSniffer warning, so I pushed a commit to fix that too. :-)

I'd say this is ready for merge! Setting to RTBC...

@alexpott @pcambra please see my suggestion re: a new 2.x branch and 2.0.0 release above! Curious what you think about that.

I would like to propose that we create a new 2.x branch for these changes, since support is being dropped for older Drupal core versions, and move forward with a 2.0.0 semantic versioning release, without the -beta designation.

What do you think @alexpott @pcambra?

I'm reviewing this now...

I see that "Bot run #11-137198" has already been merged into 8.x-1.x: https://git.drupalcode.org/project/entity_reference_validators/-/commit/...

@pierrepaul's MR makes two more changes:

1. All references to </code> are replaced with . See relevant core change record: https://www.drupal.org/node/3401941 →
2. Support is added for ^11 and dropped for ^8 || ^9.

With those changes, I'm able to include entity_reference_validators via Composer.

Tests are passing when I run them locally. Now let's see if we can get them to pass on Drupal.org GitLab...

I rebased @pierrepaul's commit onto 8.x-1.x, and committed two changes to .gitlab-ci.yml:

1. Remove _TARGET_PHP: "8.1". I think we should be testing against the default.
2. Replace _TARGET_CORE: "10.0.7" with OPT_IN_TEST_CURRENT: 1 and OPT_IN_TEST_PREVIOUS_MAJOR: 1 so we can test against Drupal 10 and 11.

.

m.stenta → made their first commit to this issue’s fork.

It looks like the exif_orientation module does not actually work on Drupal 11: 🐛 Function file_validate_image_resolution is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Active

We actually decided to remove this module in 4.x. An alternative solution has been merged into 3.x. See: https://github.com/farmOS/farmOS/pull/941

The only module we're waiting on now is jsonapi_schema.

I misspoke. We're still waiting for entity_reference_validators too: 📌 Automated Drupal 11 compatibility fixes for entity_reference_validators Needs review

Moving this to the farmOS issue queue.

We've been discussing this in the forum, and consensus seems to have landed on calling it abandoned instead of canceled. Updating this issue accordingly...

I've drafted a pull request to start thinking through the necessary code changes: https://github.com/farmOS/farmOS/pull/945

The intention is to consider this for farmOS 4.x.

I'd like to dust off our idea of adding an "abandoned" log status in 4.x.

Draft PR: https://github.com/farmOS/farmOS/pull/945

Yes! That was my hope too.

Since the will share the same module namespace (farm_map_google) I think we'll have to wait to introduce the module to core in 4.x, and deprecate the current contrib module in the meantime. More notes here:

https://github.com/farmOS/farmOS-map/issues/99#issuecomment-2761151470

https://github.com/farmOS/farmOS-map/issues/99#issuecomment-2761155869

(Linked to the wrong Drupal core issue, sorry. Corrected.)

It looks like the exif_orientation module does not actually work on Drupal 11: 🐛 Function file_validate_image_resolution is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Active

Added a link to the relevant Drupal core issue and change records in the issue description.

I can confirm that the suggestion in #21 (which is now implemented in MR70) fixes the issue for me.

In my case, the string "Moo’s Dairy Farm" was appearing in the PDF as "Mooâ??s Dairy Farm".

With the change from #21, it now appears as "Moo’s Dairy Farm".

The easiest way for me to fix this in my production deployments (while we wait for this issue) is to apply a patch via cweagans/composer-patches. Attached is a patch for this purpose (which is safer to use than a MR patch, which may be changed by anyone with push access).

Thanks for finding this solution @johanvdr!

Update: The issue I described is fixed in 🐛 Return invalid_scope error when refresh token second time. Needs work

Thank you again @bojan_dev! So good to see 6.0.0 officially released! :-)

Thank you @bojan_dev! I appreciate everything you do for this module (and Drupal generally)! I am truly grateful!

Ah ha! There is an open bug report in the Simple OAuth issue queue that is similar to this one, and the change that they propose fixes this!

🐛 Return invalid_scope error when refresh token second time. Needs work

So this may actually be a simple_oauth issue...

I just tested the change proposed by @司南 and it fixes the issue in 🐛 Refreshed access_token is missing scope with league/oauth2-server ^9 Active !

So maybe it's all the same bug in simple_oauth after all.

Setting this back to Needs Review.

@bojan_dev please see the steps to reproduce and screenshots in 🐛 Refreshed access_token is missing scope with league/oauth2-server ^9 Active - even though it is in the other module's issue queue, the fix proposed by @司南 fixes what I am seeing.

It makes me wonder if Authorization code grant types are affected too - I haven't tested them. It's very easy to test the password grant issue and see the missing scope in the Simple OAuth UI.

Was any consideration given to this comment by @bradjones1 before this change was merged??

Sorry @idebr: I see that you outlined the breaking changes in your comment #5.

Seems fine to include in the module's beta phase, but this is up to the module's maintainer

I disagree with this.

6.0.0 has been in "beta" since 2022. And we should absolutely be avoiding breaking changes, even in "beta" modules. That is what semantic versioning is for.

Please @bojan_dev can we drop the "beta" designation and adopt true semantic versioning policy moving forward?

The upgrade to league/oauth2-server 9.0 breaks the Password Grant → module: 🐛 Refreshed access_token is missing scope with league/oauth2-server ^9 Active

Has yet to be seen if this needs to be in a major or not? Guess it would depend on any major BC breaks in the library that cascade down to us.

Was any consideration given to this comment by @bradjones1 before this change was merged??

@bojan_dev PLEASE can we be more careful with these kinds of changes, and save them for major version releases?

I understand that the maintainers of simple_oauth are not responsible for downstream projects like simple_oauth_password_grant, but updating the major versions of core dependencies like this without tagging a new major version of simple_oauth provides no indication to downstream dependencies, or site admins, that there are potentially breaking changes to consider.

@司南 I'm not sure if this is related, but I just ran into this issue with the simple_oauth_password_grant module after updating to simple_oauth 6.0.0-beta10: 🐛 Refreshed access_token is missing scope with league/oauth2-server ^9 Active

There is a critical issue with this module and league/oauth2-server version 9, which I opened a separate issue for here: 🐛 Refreshed access_token is missing scope with league/oauth2-server ^9 Active

I tested the change in MR 3 and it did not fix 🐛 Refreshed access_token is missing scope with league/oauth2-server ^9 Active .

Raising the priority of this because this module seems to be broken when installed with the latest version of simple_oauth.

I tested the change in ✨ Make compatible with league/oauth2-server 9 Active (casting $uid to a string) and it does not fix this issue.

I forgot to attach the second screenshot, which shows how the same requests work properly on a site with version 8. Attaching it now...

I see that another issue was opened about supporting version 9, but it is not a bug report, so I opened this to specifically address the critical issue.

m.stenta → created an issue.

We were made maintainers of entity_reference_integrity → . I released a 2.0.0 version today with D11 support: https://www.drupal.org/project/entity_reference_integrity/releases/2.0.0 →

The only module we're waiting on now is jsonapi_schema → .

Committed.

I haven't tested this to confirm, but it seems to be the case looking at the code.

I tested this manually and it doesn't actually seem to be an issue. I created two node types, "page" and "test", both with a reference field to "page" nodes. I created 1 "page" node and 1 "test" node that references the page node. Then I created a role that only has access to view and delete "page" nodes, but not "test" nodes. When I log in as a user with that role, and I attempt to delete the "page" node, I see that I am prevented from doing so by the other node (which I do not have access to).

Nevertheless, I think it would be more correct to use accessCheck(FALSE). Even if it doesn't have an effect in the current implementation, it may in the future, or in other use-cases of this module's API.

All tests are green! We support Drupal 11 now! :-)

This probably means we should be rebuilding the relevant cache when the entity_reference_integrity_enforce.settings form is submitted.

Correction: I believe the proper approach is to add a cacheable dependency to the entity deletion form that we're altering, so that it is reset when config:entity_reference_integrity_enforce.settings changes.

I pushed a commit. Let's see if tests pass now...

It's a caching issue. The /node/1/delete page gets cached after the first time it's accessed (before the reference integrity module configuration is saved), so the second time it's accessed it still shows the "Confirm" submit button. I experienced the same thing testing manually. Notably it still prevents the user from deleting the entity when that happens - it seems to just reload the confirm form and then the expected "You can not delete this as it is being referenced by another entity" message is visible (and the submit button is gone).

This probably means we should be rebuilding the relevant cache when the entity_reference_integrity_enforce.settings form is submitted.

We might need to update our phpunit.xml configuration for PHPUnit 10. See: https://www.drupal.org/node/3365413 →

I investigated this a little, and I think the changes we need to make are minimal. We benefit from the fact that we don't maintain a copy of phpunit.xml in our repository, and instead copy the Drupal core one and run sed commands on it during the dev Docker build.

The only changes we need to make (I think) are:

- Remove the line for setting BROWSERTEST_OUTPUT_DIRECTORY, which appears to be no longer necessary: https://github.com/farmOS/farmOS/blob/cdfe6caf8a74cfd5bed574824f74a115b0...
- Remove the --verbose --debug flags from all places where we run phpunit.

There's one failing test...

    Enforced Integrity (Drupal\Tests\entity_reference_integrity_enforce\Functional\EnforcedIntegrity)
     ✘ Node integrity
       ┐
       ├ Behat\Mink\Exception\ExpectationException: An element matching css ".form-submit" appears on this page, but it should not.
       │
       │ /builds/project/entity_reference_integrity/vendor/behat/mink/src/WebAssert.php:888
       │ /builds/project/entity_reference_integrity/vendor/behat/mink/src/WebAssert.php:492
       │ /builds/project/entity_reference_integrity/modules/entity_reference_integrity_enforce/tests/src/Functional/EnforcedIntegrityTest.php:84
       │ /builds/project/entity_reference_integrity/modules/entity_reference_integrity_enforce/tests/src/Functional/EnforcedIntegrityTest.php:50
       ┴

Setting this to "Needs work"... I'll see if I can figure out what's going wrong...

Opened an MR against the new 2.x branch that makes the remaining changes. Let's see if tests pass... (thanks to 📌 Add .gitlab-ci.yml to run tests and linting Active ).

Rebased MR11 onto the new 2.x branch. Some of the changes were already taken care of in 📌 Add .gitlab-ci.yml to run tests and linting Active , so now it's just declaring ^11 support in the *.info.yml files (I amended the commit to add it to the enforce sub-module too, which didn't have core_version_requirement previously.

PS: We should probably set up CI so running tests would become easier.

This is done! 📌 Add .gitlab-ci.yml to run tests and linting Active

So let's see how the tests go...

m.stenta → made their first commit to this issue’s fork.

m.stenta → changed the visibility of the branch 8.x-1.x to hidden.

Release 8.x-1.3: https://www.drupal.org/project/entity_reference_integrity/releases/8.x-1.3 →

Published CR: https://www.drupal.org/node/3509671 →

Merged the first commit into 8.x-1.x, which simply updates the @deprecated comment to meet phpcs standards and reference this issue and the CR.

Draft CR: https://www.drupal.org/node/3509671 →

I think we can refactor the tests in 1.x

On second thought, I'm going to keep the 1.x changes as minimal as possible, to reduce the risk of introducing issues for current deployments. I will only update the @deprecated comment in 1.x.

I've drafted all the other changes necessary for 2.x.

I'll draft a change record, merge the @deprecated comment update into 8.x-1.x, tag 8.x-1.3, then rebase the rest onto a new 2.x branch.

Investigate and refactor any remaining code that uses EntityReferenceDependencyManager internally.

I think we can refactor the tests in 1.x, but other instances should probably wait until 2.x since they will change class constructor definitions and that could be a breaking change.

m.stenta → created an issue.

I am also working on fixing cspell, phpcs, and phpstan errors so that all our tests are green moving forward. Some fixes require dropping support for Drupal 9, so I am going to open a new 2.x branch before I merge this.

Here is the relevant change record which describes the recommended approach: https://www.drupal.org/node/3000490 →

// Getting a list of hook implementors is as simple as making use of the $module variable.
$implementors = [];
$this->moduleHandler->invokeAllWith($hook, function (callable $hook, string $module) use (&$implementors) {
  // There is minimal overhead since the hook is not invoked.
  $implementors[] = $module;
});

The purpose of this test is to ensure that entity_reference_integrity_enforce's implementation of hook_entity_predelete() comes before that of the comment module (to confirm that the code in hook_module_implements_alter() is doing its job).

I will refactor the test to make this more clear.

Source: #2831346: hook_entity_predelete is too late to throw an exception →

Renaming this issue to specifically focus on removing the deprecated method.

There is no file called src/Event/ReferencingEntitiesAlterEvent.php in this project.

@rajghai I don't understand how you encountered this error, nor how you created this patch. It is making changes to code that does not exist.

Closing this as "works as designed", but reopen if I'm misunderstanding something.

I'm going to close this as "works as designed", but if anyone still wants this feel free to reopen.

I'm going to close this as "works as designed", but if anyone wants to resurrect the discussion feel free to reopen.

Personally I think the fact that accessCheck(TRUE) is used in EntityReferenceIntegrityEntityHandler::referentialEntityQuery() is a bug. I opened this issue to change it to accessCheck(FALSE): 🐛 Disable access checking in EntityReferenceIntegrityEntityHandler::referentialEntityQuery() Active

I'm going to close this in favor of that issue, if that's OK. I can't see any reason why someone would want an option to set accessCheck(TRUE), so I don't think we'll need to make this configurable.

Maybe I'm misunderstanding, but the purpose of this module is to prevent orphaned references (to enforce referential integrity). Allowing enforced integrity to be turned off at a bundle level would only make it easier for these orphaned references to occur. From a purist standpoint, there probably shouldn't even be the option to enable this on a per entity type basis.

@coaston Your issue sounds like a data model problem, and working around that would just result in orphaned entities. You would be better off developing a workflow or custom business logic to allow you to delete the things you need and clean up all the references at the same time.

I'm going to close this as "won't fix" unless there's a good argument for allowing orphaned entity references that I'm missing.

I'm not sure if this should be considered a "breaking change". Technically it is a bug that allows entities to be deleted that shouldn't be.

In either case, I'm planning on starting a 2.x branch to drop support for Drupal 9 and add support for Drupal 11, so I'll make this change in there so it's part of a 2.0.0 release.

m.stenta → created an issue.

It's still missing the core_version_requirement for the entity_reference_integrity_enforce module to be compatible.

I committed this directly to the 8.x-1.x branch: https://git.drupalcode.org/project/entity_reference_integrity/-/commit/7...

In reviewing this issue, it looks like we can simply delete the entire testHookWeightLegacy() method, since the testHook() tests the same thing, using a method that exists.

I plan to start a new 2.x branch that drops support for Drupal 9, so we can remove testHookWeightLegacy() in that branch and simplify all of this.

Not a bad feature, but I'd imagine this either belongs in a sub module or another contrib so folks have the option of using this or not?

I tend to agree with @sam152 on this. I can see how this might be necessary for specific use-cases, but IMO this is outside the scope of this module, which should only be focused on maintaining referential integrity (https://en.wikipedia.org/wiki/Referential_integrity).

Anything beyond that should be handled in a separate custom/contrib module to encapsulate that specific business logic.

If anything is necessary to make that possible in this module (eg: a generic event), we can consider that. But I don't think we should add more logic to this module that is specific to nodes and the published status.

Closing this as "won't fix".

m.stenta → created an issue. See original summary → .

I'd like to dust off our idea of adding an "abandoned" log status in 4.x. This came up again on a recent dev call and there was broad support (in addition to those who expressed interest in the forum: https://github.com/farmOS/farmOS/pull/782).

Merged.

Merged.

Decide if MigrateBatchExecutable needs to be restored.

It may be that the benefits of removing MigrateBatchExecutable (less to maintain and keep in sync with migrate_tools) are enough to justify not restoring it, and the issues caused by its removal can be fixed in other ways.

We will probably work around this issue in farmOS in the short term, but adding our own explicit call to \Drupal::messenger()->addMessage(). The only concern I have is that migrate_source_ui will restore MigrateBatchExecutable in a subsequent release and then we'll have *double* messages. It would be good to coordinate to avoid that.

I discovered another issue that is a result of the removal of MigrateBatchExecutable: 🐛 StubMigrationMessage no longer used in MigrateBatchExecutable Active

m.stenta → created an issue.

Thanks @pcambra for releasing 1.0.6 of inspire_tree → w/ D11 support! https://www.drupal.org/project/inspire_tree/releases/1.0.6 →

@generalredneck made some suggestions in https://github.com/farmOS/farmOS/issues/653#issuecomment-2622888313 with regard to removing our dependency on composer-merge-plugin. It might be worth spinning off a dedicated issue to think that through, as a potential priority for 4.x.