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

Bug: Can't Rewrite System Validation Messages #4318

Closed
devajmeireles opened this issue Feb 21, 2021 · 19 comments · Fixed by #4605
Closed

Bug: Can't Rewrite System Validation Messages #4318

devajmeireles opened this issue Feb 21, 2021 · 19 comments · Fixed by #4605
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@devajmeireles
Copy link

Describe the bug
When I insert custom array of validation messages in the file: app/Language/en/Validation.php the system does not overwrite the system messages with my new messages, this only happens if the file: system/Language/en/Validation.php does not exist or be renamed.

CodeIgniter 4 version
4.1.1

Affected module(s)
Language System - I seen that only work if I remove (or rename) system/Language/en/Validation.php

Expected behavior, and steps to reproduce if appropriate
Inserting a array of Validation Messages in app/Language/en/Validation.php, the system continue to be using the system core Validation messages.

Context

  • Docker (Apache + PHP 7.4 + MariaDB)
@devajmeireles devajmeireles added the bug Verified issues on the current code behavior or pull requests that will fix them label Feb 21, 2021
@devajmeireles
Copy link
Author

  1. The message at app/Language/en/Validation.php:
    https://puu.sh/HiNRD/b60d17088a.png

  2. Generating the error who will use the same validation rule is_not_unique:
    https://puu.sh/HiNPa/faa98c124b.png

@devajmeireles
Copy link
Author

@MGatner Did you already seen something about that?

@sfadschm
Copy link
Contributor

I just tried myself and cannot confirm the issue.
What are your locale settings in the CI config?

@devajmeireles
Copy link
Author

I just tried myself and cannot confirm the issue.
What are your locale settings in the CI config?

defaultLocale = 'en';

@sfadschm
Copy link
Contributor

sfadschm commented Mar 3, 2021

This should have been handled in #3254 I think.
Maybe @samsonasik can take a look?

@samsonasik
Copy link
Member

If you can provide faiilng test case case PR, that would be easier to resolve the issue. If you have a way to resolve the issue, please do as well, thank you.

You can start from this https://github.com/codeigniter4/CodeIgniter4/blob/fd6feff3838777d50a3c84d87b68bdc2459123bd/CONTRIBUTING.md#how-to-guide

@sfadschm
Copy link
Contributor

sfadschm commented Mar 3, 2021

The test in #4387 fails successfully (besides the image stuff), so overwriting should be working.

1) CodeIgniter\Language\LanguageTest::testPrioritizedLocator
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'The framework needs the following extension(s) installed and loaded: {0}.'
+'Test'

@paulbalandan
Copy link
Member

If I understand correctly, the test case demonstrates that app prioritisation is working as intended? If that's the case, then there are unseen variables causing this issue's problem.

@sfadschm
Copy link
Contributor

sfadschm commented Mar 3, 2021

If I understand correctly, the test case demonstrates that app prioritisation is working as intended?

Correct.

@devajmeireles
Copy link
Author

@samsonasik

The message in app/Language/en/Validation.php:
https://puu.sh/HlRIi/20c553fe1a.png

The message displayed:
https://puu.sh/HlRII/f585a9accd.png

I don't know how to use the test system correctly. But analyzing it manually, the rewriting of system validation messages only occurs when I rename the system validation file to any other name.

I also noticed that the app/Language/en/Validation.php is called first, but I don't know why it still gets messages from the system file instead of the application file.

@devajmeireles
Copy link
Author

Can close this topic. I can't help but it is a bug. I will change the system files to correct the failure.

@paulbalandan
Copy link
Member

I think I know now where is the problem.

@paulbalandan
Copy link
Member

@ajmeireles Can you do this little tweaking for me in FileLocator::search? See diff below:

		foreach ($this->getNamespaces() as $namespace)
		{
			if (isset($namespace['path']) && is_file($namespace['path'] . $path))
			{
				$fullPath = $namespace['path'] . $path;
+				$fullPath = realpath($fullPath) ?: $fullPath;

				if ($prioritizeApp)
				{
					$foundPaths[] = $fullPath;
				}
				elseif (strpos($fullPath, APPPATH) === 0)
				{
					$appPaths[] = $fullPath;
				}
				else
				{
					$foundPaths[] = $fullPath;
				}
			}
		}

@paulbalandan
Copy link
Member

https://puu.sh/HlRO3/88a52c35b2.png

This snip gives the reason why it's failing. strpos on APPPATH on a relative link will likely fail. So the app file will be the first in the array, followed by the system. This should be the other way around. This order is necessary because Language::requireFile() uses array_replace_recursive on the first array with replacements from the second. Since the first is the app version, it effectively is overwritten by the system version.

@sfadschm
Copy link
Contributor

sfadschm commented Mar 4, 2021

Interesting. Why does overriding work in the tests though?

@paulbalandan
Copy link
Member

Simple. Because testing happens outside of Composer world.

In our unit tests, the paths do not arise from vendor/ but directly from system and app. So strpos check on these paths is straightforward: system/Language/en/** and app/Language/en.

When required as a vendor by Composer, the paths change. It is now relative to Composer. vendor/composer/../../app/Language/en/**. So the strpos($path, APPPATH) === 0 check will fail here.

@sfadschm
Copy link
Contributor

sfadschm commented Mar 4, 2021

Oh, okay. Never thought about composer 😆
Good catch!

@sfadschm
Copy link
Contributor

sfadschm commented Mar 8, 2021

@ajmeireles Could you test Paul's suggestion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants