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

Add filters and sorting on date of check in Report #106

Merged
merged 8 commits into from
Oct 17, 2021

Conversation

atigiti
Copy link
Contributor

@atigiti atigiti commented Oct 12, 2021

I tried to add filters functionality and also the sorting on date of check in Report

@sypets
Copy link
Owner

sypets commented Oct 13, 2021

@atigiti Thank you for your PR. I tested it and like the functionality. In the actions , you can see that some checks failed (you can also see that in the PR).

Since these are minor issues (coding guidelines, phpstan functional tests), I already pushed some fixes.

In the future you can perform the checks locally yourself, and in the case of coding guidelines (CGL) even fix them locally.

Please see the file CONTRIBUTING.md for more info.

Also, I adhere to TYPO3 core best practices as much as possible, core also enforces CGL checks and uses phpstan.

Example: phpstan complains about this but I also prefer to add type hints as much as possible, e.g.

-    public function setStorageKey($storageKey)
+    public function setStorageKey(string $storageKey): void

I will give the patch a more thorough review next, the above are more general things.

@atigiti
Copy link
Contributor Author

atigiti commented Oct 13, 2021

Thank you for your response, I will start to run the local tests before the PR, thanks also for changing the getBrokenLinks signature!

@atigiti
Copy link
Contributor Author

atigiti commented Oct 13, 2021

i'm trying also to add a new Tab in the info module, for Manage the exclusions links.

Copy link
Owner

@sypets sypets left a comment

Choose a reason for hiding this comment

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

Great change, I especially like the possibility to filter for URL. I tested and it works if sorting is changed and also works with pagination.

I made some comments. If you can get these fixed, that would be great.

I already mentioned the phpstan, CGL, test things - that might be something to do in future PR.

Resources/Private/Templates/Backend/ReportTab.html Outdated Show resolved Hide resolved
Resources/Private/Templates/Backend/ReportTab.html Outdated Show resolved Hide resolved
Classes/View/BrofixReport.php Outdated Show resolved Hide resolved
Classes/View/BrofixReport.php Outdated Show resolved Hide resolved
Classes/View/BrofixReport.php Outdated Show resolved Hide resolved
Classes/View/BrofixReport.php Outdated Show resolved Hide resolved
Classes/View/BrofixReport.php Outdated Show resolved Hide resolved
Resources/Private/Templates/Backend/ReportTab.html Outdated Show resolved Hide resolved
Classes/BackendSession/BackendSession.php Outdated Show resolved Hide resolved
@atigiti
Copy link
Contributor Author

atigiti commented Oct 13, 2021

@atigiti Thank you for your PR. I tested it and like the functionality. In the actions , you can see that some checks failed (you can also see that in the PR).

Since these are minor issues (coding guidelines, phpstan functional tests), I already pushed some fixes.

In the future you can perform the checks locally yourself, and in the case of coding guidelines (CGL) even fix them locally.

Please see the file CONTRIBUTING.md for more info.

Also, I adhere to TYPO3 core best practices as much as possible, core also enforces CGL checks and uses phpstan.

Example: phpstan complains about this but I also prefer to add type hints as much as possible, e.g.

-    public function setStorageKey($storageKey)
+    public function setStorageKey(string $storageKey): void

I will give the patch a more thorough review next, the above are more general things.

i have a question please, how can use the phpstan in typo3 project ???

@sypets
Copy link
Owner

sypets commented Oct 13, 2021

@atigiti It depends - if you want to use it to test brofix or set it up for your project

normally, you can do this (after you setup the phpstan.neon):

composer require phpstan/phpstan
php vendor/bin/phpstan analyse --no-progress --no-interaction --memory-limit 4G  -c phpstan.neon

For brofix however, the test suite is setup to run via runTests.sh using this instruction: https://docs.typo3.org/m/typo3/reference-coreapi/10.4/en-us/Testing/ExtensionTesting.html
For runTests.sh to work you need to install Docker and Docker compose. This may seem like overkill for phpstan but you can also run the functional tests that way which require a database to be setup etc. and for this the Docker container is handy.

You must first do the equivalent of composer install:

In the brofix directory:

Build/Scripts/runTests.sh -p 7.4 -s composerInstallMax

After that, you can call phpstan with

composer ci:phpstan


Sorry, if this seems a bit complicated. I may still change things in the future.

You can also look here:


You can also come to the Slack workspace and write me a direct message to @sybille. Patrick knows how to get access to Slack.
`

@atigiti
Copy link
Contributor Author

atigiti commented Oct 14, 2021

@atigiti It depends - if you want to use it to test brofix or set it up for your project

normally, you can do this (after you setup the phpstan.neon):

composer require phpstan/phpstan
php vendor/bin/phpstan analyse --no-progress --no-interaction --memory-limit 4G  -c phpstan.neon

For brofix however, the test suite is setup to run via runTests.sh using this instruction: https://docs.typo3.org/m/typo3/reference-coreapi/10.4/en-us/Testing/ExtensionTesting.html For runTests.sh to work you need to install Docker and Docker compose. This may seem like overkill for phpstan but you can also run the functional tests that way which require a database to be setup etc. and for this the Docker container is handy.

You must first do the equivalent of composer install:

In the brofix directory:

Build/Scripts/runTests.sh -p 7.4 -s composerInstallMax

After that, you can call phpstan with

composer ci:phpstan

Sorry, if this seems a bit complicated. I may still change things in the future.

You can also look here:

You can also come to the Slack workspace and write me a direct message to @sybille. Patrick knows how to get access to Slack. `

Thank you for your help!

@atigiti
Copy link
Contributor Author

atigiti commented Oct 14, 2021

in my last commit, i tried to fix all point in the code that you mentioned after your code check, i also run the tests and phpstan for code review, i hope thaht will be helpful!

@sypets
Copy link
Owner

sypets commented Oct 16, 2021

@atigiti I pushed a fix for the CGL issue

The pipepline has run executing the tests: https://github.com/sypets/brofix/actions/runs/1341403243

image

see Actions

You can fix coding guideline issues locally: Build/Scripts/runTests.sh -s cgl
You can also run some basic tests locally with the script: Build/Scripts/ci.sh

see https://github.com/sypets/brofix/blob/master/CONTRIBUTING.md

These commands are still work-in-progress, will optimize and then update CONTRIBUTING.md

composer.json Outdated Show resolved Hide resolved
@sypets sypets merged commit 83fa906 into sypets:master Oct 17, 2021
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

2 participants