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

Revert unneeded changes from "Fixes for nested multipliers (#59)" #92

Closed
wants to merge 1 commit into from
Closed

Revert unneeded changes from "Fixes for nested multipliers (#59)" #92

wants to merge 1 commit into from

Conversation

jtojnar
Copy link
Collaborator

@jtojnar jtojnar commented Feb 18, 2024

This partially reverts commit 39725d3, only keeping the part relevant for nested support and a PHPUnit deprecation fix.

The commit introduced a regression in testGroupManualRenderWithButtons so this will minimize the changes that could affect it.

  • Latte 2 macros are trivial to migrate and unclearly named, no need to keep them for BC.
  • The onSuccess handlers were already fixed in ed96ba0.
  • testGroupManualRenderWithButtons look like remnants of debugging effort, they do not fix the test.

For reference, the reverted change is the following commit that was squashed into 39725d3, except for the change to testSendNested: 2c33de2

@jtojnar jtojnar marked this pull request as draft February 19, 2024 01:21
@f3l1x
Copy link
Member

f3l1x commented Feb 23, 2024

Tests are still failing. :(

@jtojnar
Copy link
Collaborator Author

jtojnar commented Feb 23, 2024

Yes, the primary reason is that webchemistry/testing-helpers is broken with latest Nette.

If you merge #96, that will fix it, and only two test failures will remain:

This pull request does not fix any tests, only cleans the code a bit. But spending most of the last weekend trying to decipher the changes in #59, I am now convinced the PR should be reverted wholesale, and the fix reimplemented.

I will try looking into it and into fixing #83 this weekend.

@f3l1x
Copy link
Member

f3l1x commented Feb 23, 2024

Thanks. The best thing would be drop webchemistry/testing-helpers at all. :-/

@jtojnar
Copy link
Collaborator Author

jtojnar commented Feb 23, 2024 via email

This partially reverts commit 39725d3,
only keeping the part relevant for nested support and a PHPUnit deprecation fix.

The commit introduced a regression in `testGroupManualRenderWithButtons`
so this will minimize the changes that could affect it.

- Latte 2 macros are trivial to migrate and unclearly named, no need to keep them for BC.
- The onSuccess handlers were already fixed in ed96ba0.
- `testGroupManualRenderWithButtons` look like remnants of debugging effort, they do not fix the test.

For reference, the reverted change is the following commit that was squashed
into 39725d3, except for the change to `testSendNested`:
2c33de2
@jtojnar
Copy link
Collaborator Author

jtojnar commented May 14, 2024

I became a maintainer of testing-helpers and fixed it there.

As for #59, I think it will be easier to revert it completely and re-implement it from scratch. Doing that in #110.

@jtojnar jtojnar closed this May 14, 2024
@jtojnar jtojnar deleted the partial-revert-59 branch May 14, 2024 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants