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 routes regular expression for windows #729

Closed
wants to merge 1 commit into from
Closed

fix routes regular expression for windows #729

wants to merge 1 commit into from

Conversation

GeorgKott
Copy link
Contributor

@GeorgKott GeorgKott commented May 4, 2023

Hello! I had a problem with change route file while setup Shield on Windows. It was because i have "\r\n", not "\n". Route file has not changed.
This is my fix. What do you thin about this, thank you

@datamweb
Copy link
Collaborator

datamweb commented May 4, 2023

Hi,
I haven't seen the code yet, but I have no problem installing on Windows.
So I expect you to explain how I can reproduce your problem?

@GeorgKott
Copy link
Contributor Author

GeorgKott commented May 4, 2023

Did you try to start tests on windows? I have problems with TestAddAfter

 ./vendor/bin/phpunit --no-coverage tests                                         
PHPUnit 9.6.7 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.14
Configuration: \PhpstormProjects\shield\phpunit.xml.dist
Random Seed:   1683145024

...............................................................  63 / 391 ( 16%)
............................................................... 126 / 391 ( 32%)
............................................................... 189 / 391 ( 48%)
............................................................... 252 / 391 ( 64%)
............................................................... 315 / 391 ( 80%)
..................FFF.......................................... 378 / 391 ( 96%)
.............                                                   391 / 391 (100%)

Nexus\PHPUnit\Extension\Tachycardia identified these 6 slow tests:
+---------------------------------------------------------------------------------+---------------+-------------+
| Test Case                                                                       | Time Consumed | Time Limit  |
+---------------------------------------------------------------------------------+---------------+-------------+
| Tests\\Unit\\UserIdentityModelTest::testForceMultiplePasswordReset              | 00:00:00.61   | 00:00:00.50 |
| Tests\\Controllers\\RegisterTest::testRegisteredAndSessionExpiredAndLogin       | 00:00:00.60   | 00:00:00.50 |
| Tests\\Controllers\\RegisterTest::testRegisteredButNotActivatedAndLogin         | 00:00:00.55   | 00:00:00.50 |
| Tests\\Controllers\\RegisterTest::testRegisteredButNotActivatedAndRegisterAgain | 00:00:00.55   | 00:00:00.50 |
| Tests\\Controllers\\RegisterTest::testRegisterActionSuccess                     | 00:00:00.53   | 00:00:00.50 |
| Tests\\Controllers\\RegisterTest::testRegisterRedirectsToActionIfDefined        | 00:00:00.53   | 00:00:00.50 |
+---------------------------------------------------------------------------------+---------------+-------------+


Time: 00:18.343, Memory: 52.00 MB

There were 3 failures:

1) Tests\Commands\Setup\ContentReplacerTest::testAddAfter
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@

 $routes->get('/', 'Home::index');

-service('auth')->routes($routes);
-
 $routes->group('admin', static function ($routes) {
     $routes->get('users', 'Admin\Users::index');
     $routes->get('blog', 'Admin\Blog::index');

\PhpstormProjects\shield\tests\Commands\Setup\ContentReplacerTest.php:109

2) Tests\Commands\Setup\ContentReplacerTest::testAddBefore
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 #Warning: Strings contain different line endings!
 '<?php

 namespace App\Controllers;
@@ @@
 {
     public function initController(RequestInterface $request, ResponseInterface $response, LoggerInterface $logger)
     {
-        $this->helpers = array_merge($this->helpers, ['auth', 'setting']);
-
+        $this->helpers = array_merge($this->helpers, ['auth', 'setting']);
+
         // Do Not Edit This Line
         parent::initController($request, $response, $logger);
     }
 }'

PhpstormProjects\shield\tests\Commands\Setup\ContentReplacerTest.php:151

3) Tests\Commands\Setup\ContentReplacerTest::testReplace
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 namespace Config;

+use CodeIgniter\Config\BaseConfig;
 use CodeIgniter\Shield\Models\UserModel;

 class Auth extends \CodeIgniter\Shield\Config\Auth
 {'

PhpstormProjects\shield\tests\Commands\Setup\ContentReplacerTest.php:47

FAILURES!
Tests: 391, Assertions: 827, Failures: 3.

It is because in windows typical situation that newline is "\r\n", not "\n"

@paulbalandan
Copy link
Member

It seems you configured your Git for Windows client to use CRLF instead of LF. Try this:

git config --global core.autocrlf false
git config --global core.eol lf

Then reload your IDE.

@GeorgKott
Copy link
Contributor Author

GeorgKott commented May 4, 2023

This is a solution to the problem, thank you.

However, there are no requirements in the Shield installation guide stating that you need LF instead of CRLF. There is also no requirement to configure git in any specific way. And the user might very well have a configuration with CRLF

@datamweb
Copy link
Collaborator

datamweb commented May 4, 2023

@GeorgKott Shield is an official package, so we assume that all the rules in CI4 apply to Shield as well.
The necessary explanations are here.

To be honest I'm not sure we need to explain all the details in the Shield docs.

If you think it needs clarification, I'm fine with sending a PR for the docs.

@paulbalandan
Copy link
Member

@GeorgKott If you are working with open source PHP projects that caters all OS, then it is expected that you have a unified line ending, which is LF. When you install Git for Windows, you should have seen that option for line endings. This is made so that the project is interoperational with all OS.

@GeorgKott
Copy link
Contributor Author

Thank you for your comments. I closed this pull request

@GeorgKott GeorgKott closed this May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants