[PHPUnit] | GitHub Action tests are missing locale keys and passing for wrong reasons

[Tribunal33]

Description:

• Include specific context details that have been discovered that surround this developer task.
• Link related tickets

I'm not well versed in PHP so this might not be an issue but I've noticed that the GitHub Actions runs are showing missing locale keys for Validator tests. I think the reason is that it passes is because it is asserting false but I don't think it is failing for the right reasons. Could we take a look at these validator AssertFalse tests so they aren't throwing so many missing keys.

GitHub Actions

Sample code where they are being tested in the PHPunit Tests

Steps to test the changes:

1. Run the phpunit tests and it should pass validator tests with correct validation rule.

Solution: What the proposed solutions are and additional affected areas
Either we can remove these validator checks or we can add a rule so that it passes without missing locale keys. I gave a reason for what is happening. I'm not sure if this is expected but when going through these logs it seems like the test isn't doing what it should be doing.

What application are you using?
OJS 3.5 and main but GitHub Actions all PHPunit test runs.

Additional information
Please add any screenshots, videos or other information we can use to understand this dev task report.

After parsing the key with the rule it returns with a key that isn't available so it concatenates to an unknown gibberish key like "validation.values.value.anything else". This will return false because of a missing key and still assert true because the test is expected to fail. I am not sure if the intent of this isValid method is correctly being asserted by this test

PRs

Stable-3.5.0

ojs --> pkp/ojs#4966
ops --> pkp/ops#1043
omp --> pkp/omp#2056
pkp-lib --> #11597
ojs --> pkp/ojs#4978 [TEST ONLY]
omp --> pkp/omp#2075 [TEST ONLY]
ops --> pkp/ops#1062 [TEST ONLY]

main

pkp-lib --> #12016
ojs --> pkp/ojs#5174
omp --> pkp/omp#2186
ops --> pkp/ops#1154
staticPages --> pkp/staticPages#89
webFeed --> pkp/webFeed#16
googleScholar --> pkp/googleScholar#33
customBlockManager --> pkp/customBlockManager#92
googleAnalytics --> pkp/googleAnalytics#62
tinymce --> pkp/tinymce#97
browse --> pkp/browse#15
crossref-ojs --> pkp/crossref-ojs#78
crossref-ops --> pkp/crossref-ops#57

PR Impacts [main (upcoming 3.6)]:

What has changed

• const PKP_STRICT_MODE has been removed
• The class_alias is removed so form now on need to use fully qualified namespace for classes
• Unit test will always run in strict mode backed by specific const PKP_PHPUNIT_STRICT_MODE in phpunit-bootstrap.php
• Deprecated instantiate has been removed.
• PluginRegistry::deprecatedInstantiatePlugin has been removed so plugin instantiation using deprecated approach will not work
• Locale missing log will be only available when the app running in strict mode
• All the dot notation class paths (e.g. classes::classes.submission.Submission) specifically related to filters has been updated with proper namespace path

For Plugin development

• Plugin should follow the proper namespace path
• Plugins not following proper namespace path will need to have wrapper index.php in the plugin path
• Plugin not following proper namespace and not having a wrapper index.php will be failed to initialize

NOTE : require to run the migration PKP\migration\upgrade\v3_6_0\I11583_ClassNamespaceFromDotNotationClassPath as

php lib/pkp/tools/migration.php "PKP\\migration\\upgrade\\v3_6_0\\I11583_ClassNamespaceFromDotNotationClassPath" up

[asmecher]

I've confirmed that we'll have a tough time working around this without fighting Laravel. The code that causes it is here:

https://github.com/laravel/framework/blob/5ad0a1c079f8a8dda603c97333864b7ef4b11d20/src/Illuminate/Validation/Concerns/FormatsMessages.php#L479..L483

Essentially it depends on the fact that Laravel's translator returns the string unchanged when a locale key isn't present, which is possible since Laravel doesn't use synthetic locale keys. We do, so this will never work for us (until we move away from monolingual translation files).

@touhidurabir, I think I have a good, non-invasive solution:

• Make the error message in the log about missing locale keys dependent on being in strict mode (grep for PKP_STRICT_MODE; see the strict option in config.inc.php). If you're not in strict mode, no error/warning should be logged.
• See if you can make the CI tests operate under strict mode -- not sure whether this will be a big time sink or not, but if not, let's do it. It's a good idea beyond just this issue.
• Have the validator tests undefine the PKP_STRICT_MODE constant before running the tests that create the spurious log entries.

[touhidurabir]

@asmecher I think it's kind of doable except the part Have the validator tests undefine the PKP_STRICT_MODE constant before running the tests that create the spurious log entries. as PKP_STRICT_MODE set in the PKPApplication::__construct where application instance is created as part of bootstrapping process . please correct me if I am misunderstanding something here .

However , we have a other options like PKPContainer::runningUnitTests to check test running status and use that to suppress any translation missing log during the unit test .

But still , if installation run in the PKP_STRICT_MODE as true e.g. in the strict mode , it will log meaning less translation missing log when user provide invalid data which seems bit misleading . what do you think about that ?

[asmecher]

Huh, I could've sworn there was an undefine function -- but at a glance it's impossible to undefine PHP constants! I'd suggest moving the PKP_STRICT_MODE check into a function of the PKPApplication class -- but rather than have you go through and change all those deprecated class definitions, I'll go through and remove those now. It's a good time now that 3.5 is out. They were introduced with 3.4.