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

[php8-compat] Partial Fix of hook tests for php8 #20545

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

seamuslee001
Copy link
Contributor

Overview

This partially fixes some test failures on testing hooks

Before

Test fails

After

Test fails but not for these reasons

ping @eileenmcnaughton @totten @colemanw @demeritcowboy

@civibot
Copy link

civibot bot commented Jun 8, 2021

(Standard links)

@civibot civibot bot added the master label Jun 8, 2021
@seamuslee001 seamuslee001 force-pushed the hook_test_fix_part1 branch from afd93a0 to 6514beb Compare June 8, 2021 01:05
@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@@ -26,8 +26,10 @@
<td class="label">{$form.description.label}
{if $action == 2}{include file='CRM/Core/I18n/Dialog.tpl' table='civicrm_option_group' field='description' id=$id}{/if}</td><td>{$form.description.html}</td>
</tr>
{if !empty($form.name)}
<td class="label">{$form.name.label}</td>
<td>{$form.name.html}</td></tr>
Copy link
Member

Choose a reason for hiding this comment

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

There's a closing </tr> tag in this if block that doesn't match anything else inside the block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @colemanw have fixed that up now hopefully

Copy link
Contributor

Choose a reason for hiding this comment

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

Look up at line 20

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @demeritcowboy fixed it properly now I would say

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes looks good. I can't see where this field comes up though - I think name is not exposed anymore.

@seamuslee001 seamuslee001 force-pushed the hook_test_fix_part1 branch from 6514beb to ee65c22 Compare June 9, 2021 00:13
@seamuslee001 seamuslee001 force-pushed the hook_test_fix_part1 branch from ee65c22 to b0e06ce Compare June 9, 2021 00:18
@@ -38,22 +38,22 @@
<span class="description">{ts}A "string value" or regular expression to be redacted (replaced).{/ts}</span>
</td>
</tr>
{else}
{elseif !empty($form.label)}
Copy link
Contributor

Choose a reason for hiding this comment

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

This would also hide financial_account_id when it didn't before, since despite the indenting fin id is within this block. But I'm not sure what screen that comes up on. Clicking around the related screens looks ok.

@demeritcowboy
Copy link
Contributor

Clicked around and didn't see any obvious issues. Removing possibly never-used stuff would be out of scope.

@demeritcowboy demeritcowboy merged commit 9eb7f5b into civicrm:master Jun 9, 2021
@seamuslee001 seamuslee001 deleted the hook_test_fix_part1 branch June 9, 2021 02:19
@seamuslee001
Copy link
Contributor Author

thanks @demeritcowboy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants