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] VirtualPage #1602

Merged
merged 13 commits into from
Feb 3, 2021
Merged

[fix] VirtualPage #1602

merged 13 commits into from
Feb 3, 2021

Conversation

ibelar
Copy link
Contributor

@ibelar ibelar commented Feb 1, 2021

Fix for #1587

The callback Callback::set() method needs to be called prior to VirtualPage::getHtml() method in order for other's callback inside VirtualPage to be execute properly.

BC Break

  • Property $fx was remove and no longer necessary because Callback::set() method is called during VirtualPage::set() and not during VirtualPage::getHtml() as before. This is to ensure that Callback::set() is always run.

    Therefore, using DI to set $fx property viaVirtualPage::addTo($app, ['fx' => function(){}])is no longer supported.

  • Virtual::Page::set() only accept \Closure instead of callable as before. You must convert your callable into Closure via \Closure::fromCallable()

Fix for #1587

Callable for VirtualPage should be called prior to VirtualPage::getHtml() in order to make sure nested callback (Lookup) can be reach.
src/VirtualPage.php Outdated Show resolved Hide resolved
@ibelar ibelar added the RTM label Feb 1, 2021
@ibelar ibelar added the BC-break label Feb 2, 2021
src/VirtualPage.php Outdated Show resolved Hide resolved
src/VirtualPage.php Outdated Show resolved Hide resolved
Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

LGTM

@ibelar ibelar merged commit 32f18d7 into develop Feb 3, 2021
@ibelar ibelar deleted the fix/virtual-page branch February 3, 2021 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants