Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix form submission, when form block is inside another block #1107

Conversation

sukhwinder-somar
Copy link
Contributor

@sukhwinder-somar sukhwinder-somar commented Oct 11, 2023

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Please change to this instead:

public function handleElement()
{
    $id = $this->owner->getRequest()->param('ID');

    if (!$id) {
        user_error('No element ID supplied', E_USER_ERROR);
        return false;
    }

    /** @var DataObject $elementOwner */
    $elementOwner = $this->owner->data();

    if (!$elementOwner->hasExtension(ElementalAreasExtension::class)) {
        user_error(get_class($elementOwner) . ' doesnt have the ElementalAreasExtension applied', E_USER_ERROR);
        return false;
    }

    $elementalAreaRelations = $elementOwner->getElementalRelations();
    if (!$elementalAreaRelations) {
        user_error(get_class($elementOwner) . ' has no ElementalArea relationships', E_USER_ERROR);
        return false;
    }

    $element = $this->findElement($elementalAreaRelations, $elementOwner, $id);

    if ($element) {
        return $element->getController();
    }

    user_error('Element $id not found for this page', E_USER_ERROR);
    return false;
}

private function findElement(iterable $elementalAreaRelations, DataObject $owner, $id): ?DataObject
{
    foreach ($elementalAreaRelations as $elementalAreaRelation) {
        $elements = $owner->$elementalAreaRelation()->Elements();
        $found = $elements->filter('ID', $id)->First();

        if ($found) {
            return $found;
        }

        /** @var BaseElement $element */
        foreach ($elements as $element)             {
            if (!$element->hasExtension(ElementalAreasExtension::class)) {
                continue;
            }

            /** @var BaseElement&ElementalAreasExtension $element */
            $found = $this->findElement($element->getElementalRelations(), $element, $id);

            if ($found) {
                return $found;
            }
        }
    }

    return null;
}

This is similar to your approach, but with the following key differences:

  • Adds an additional protection against $elementOwner not having the ElementalAreasExtension applied (in the case of custom nested elements)
  • Checks for use of the ElementalAreasExtension instead of the Elements method, which allows for the relation to have any arbitrary name instead of just the one that the elemental list module uses
  • Flips the logic of findElement() somewhat, so we have no foreach loop in the handleElement() anymore, delegating all of the logic to findElement().
  • Adds appropriate type hinting to the new method

I've tested this in conjunction with dnadesign/silverstripe-elemental-userforms#78 and together they resolve the bug, so after you've made these changes I'll be happy to merge them both.

@sukhwinder-somar
Copy link
Contributor Author

sweet, I have updated the code with your changes, but I didn't test it so I'm hoping it will work as you have already tested it....

@sukhwinder-somar
Copy link
Contributor Author

sweet, I have updated the code with your changes, but I didn't tested it so I'm hoping it will work as you have already tested it....

@GuySartorelli
Copy link
Member

Please test the changes. I will merge once you have confirmed they work on your end.
The whole point of a pull request is that there are at least two people involved.

@sukhwinder-somar
Copy link
Contributor Author

Sure, then I will update you tomorrow or tonight after testing...

@sukhwinder-somar
Copy link
Contributor Author

Hi,

I have tested this and can confirm the above is working for me on First and Second Level of page.

While testing I had first and second level forms on same page and I found out If I was submitting one of them, then both of them was showing confirmation messages, which I feel looks weird, so thought I should tell you....

thanks,

@GuySartorelli
Copy link
Member

While testing I had first and second level forms on same page and I found out If I was submitting one of them, then both of them was showing confirmation messages, which I feel looks weird, so thought I should tell you....

That's a known issue, captured in dnadesign/silverstripe-elemental-userforms#50 - it was fixed recently so if you update your dependency it shouldn't happen anymore.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for fixing this.

@GuySartorelli
Copy link
Member

Ahh, sorry, I just realised this is targetting the wrong branch.
Please retarget it to the 4.11 branch. You may need to reset your commits afterward.

@sukhwinder-somar sukhwinder-somar changed the base branch from 4 to 4.11 October 13, 2023 02:27
@sukhwinder-somar sukhwinder-somar force-pushed the fix-elemental-form-submission/issue-27 branch from 59e6e4c to c205889 Compare October 13, 2023 02:32
@sukhwinder-somar
Copy link
Contributor Author

All good, thanks for all support 😊 🙏

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Oops, looks like when you reset the commits you accidentally reverted the changes from #1107 (review) as well!

@sukhwinder-somar
Copy link
Contributor Author

apologies again, I forgot to cherry pick both commits, but have added it now 😅

@GuySartorelli GuySartorelli merged commit 1e9f05c into silverstripe:4.11 Oct 13, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants