Skip to content
This repository has been archived by the owner on Aug 18, 2024. It is now read-only.

Revert the fix from #709 #731

Merged
merged 4 commits into from
Jan 12, 2022
Merged

Revert the fix from #709 #731

merged 4 commits into from
Jan 12, 2022

Conversation

pfrenssen
Copy link
Contributor

In local testing I have discovered that the fix that was proposed in #709 does not seem to be needed. The test for that PR still passes if the fix is reverted.

It might be possible I am getting a false negative due to some environment specific issues (for example, I have only tested on a Drupal 8 installation). Also it is possible that the test for the issue was not capturing the exact steps needed to replicate the bug.

Let's see what the test bot says.

It seems the test for PR #709 still passes if the fix is reverted. It might not
be needed to special case this route after all?
@pfrenssen
Copy link
Contributor Author

OK I found out why this is passing on my installation. This is actually a duplicate of a core bug: Issue #2730631: Upcast node and node_revision parameters of node revision routes. I had this patch applied.

In the meantime this has been fixed in core and released in Drupal 9.3.

Shall we remove the fix from OG since this is not the right place to fix it? If users are affected they can update to Drupal 9.3, or on older versions they can apply the patch from the core issue.

@amitaibu
Copy link
Member

Shall we remove the fix from OG since this is not the right place to fix it?

Yes, sounds right

@MPParsley MPParsley added this to the 1.0-alpha8 milestone Dec 15, 2021
@pfrenssen pfrenssen mentioned this pull request Dec 17, 2021
Copy link
Contributor

@idimopoulos idimopoulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTBC +1.

@pfrenssen pfrenssen merged commit 10802e1 into 8.x-1.x Jan 12, 2022
@pfrenssen pfrenssen deleted the revert-709 branch January 12, 2022 08:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants