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

Use strict_types #5830

Conversation

ColonelMoutarde
Copy link
Contributor

@ColonelMoutarde ColonelMoutarde commented Nov 8, 2023

Changes proposed in this pull request:

  • Little's optimisations and booleans in conditions

Pull request checklist:

  • clear commit messages
  • code manually tested

@@ -1,5 +1,7 @@
<?php

declare(strict_types=1);
Copy link
Member

Choose a reason for hiding this comment

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

Let's not make many PRs with partial changes, so let's start by deciding where we should have that in production (pros and cons) and then apply on everything we have decided (and tested!) at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any disadvantages or problems. on the contrary, the code is more robust because it does what it says.

Copy link
Member

@Alkarex Alkarex Nov 9, 2023

Choose a reason for hiding this comment

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

We will probably add those lines (it is the right time now, that we have just added PHP 7.4 typed properties), but examples of disadvantages are increased risks of crash in production, and tiny impact on performance (due to additional checks at runtime)...

app/Models/LogDAO.php Outdated Show resolved Hide resolved
@Alkarex Alkarex changed the title Little's optimisations and booleans in conditions Use strict_types Nov 9, 2023
@Alkarex Alkarex added this to the 1.23.0 milestone Nov 9, 2023
@Alkarex Alkarex added the System care Everything related to the system care label Nov 9, 2023
@Alkarex
Copy link
Member

Alkarex commented Nov 13, 2023

Let's do that in one go instead of many small PRs. Please add declare(strict_types=1); in all our files (excluding third-party libraries) so that we can start testing

cli/i18n/I18nFile.php Outdated Show resolved Hide resolved
@Alkarex
Copy link
Member

Alkarex commented Nov 16, 2023

Ah, I can see the other change you made was probably to address:

PHP Fatal error: Uncaught TypeError: addcslashes(): Argument #1 ($string) must be of type string, I18nValue given in /home/runner/work/FreshRSS/FreshRSS/cli/i18n/I18nFile.php:123

@Alkarex
Copy link
Member

Alkarex commented Nov 16, 2023

There were multiple errors under PHP 8.2 / 8.3. Fixed in 96c0c52

docker run --rm -it -e FRESHRSS_ENV=development -v $PWD:/var/www/FreshRSS freshrss/freshrss:newest bin/composer test

@Alkarex
Copy link
Member

Alkarex commented Nov 16, 2023

Many declarations were missing. Also added for phtml files. More errors fixed
36e8d01

@Alkarex
Copy link
Member

Alkarex commented Nov 16, 2023

Crashes. Needs more testing. Example:

Fatal error: Uncaught TypeError: FreshRSS_Entry::_id():
Argument #1 ($value) must be of type string, int given,
called in /var/www/FreshRSS/app/Models/Entry.php on line 82
and defined in /var/www/FreshRSS/app/Models/Entry.php:441
Stack trace: #0 /var/www/FreshRSS/app/Models/Entry.php(82): FreshRSS_Entry->_id()
#1 /var/www/FreshRSS/app/Models/EntryDAO.php(1148): FreshRSS_Entry::fromArray()
#2 /var/www/FreshRSS/app/Controllers/indexController.php(266): FreshRSS_EntryDAO->listWhere()
#3 /var/www/FreshRSS/app/views/index/normal.phtml(30): FreshRSS_index_Controller::listEntriesByContext()
#4 /var/www/FreshRSS/lib/Minz/View.php(93): include('...')
#5 /var/www/FreshRSS/lib/Minz/View.php(115): Minz_View->includeFile()
#6 /var/www/FreshRSS/app/layout/layout.phtml(73): Minz_View->render()
#7 /var/www/FreshRSS/lib/Minz/View.php(93): include('...')
#8 /var/www/FreshRSS/lib/Minz/View.php(106): Minz_View->includeFile()
#9 /var/www/FreshRSS/lib/Minz/View.php(73): Minz_View->buildLayout()
#10 /var/www/FreshRSS/lib/Minz/Dispatcher.php(59): Minz_View->build()
#11 /var/www/FreshRSS/lib/Minz/FrontController.php(61): Minz_Dispatcher->run()
#12 /var/www/FreshRSS/p/i/index.php(59): Minz_FrontController->run()
#13 {main} thrown in /var/www/FreshRSS/app/Models/Entry.php on line 441

@Alkarex Alkarex marked this pull request as draft November 16, 2023 12:35
@Alkarex Alkarex marked this pull request as ready for review November 16, 2023 12:35
Copy link
Member

@Alkarex Alkarex left a comment

Choose a reason for hiding this comment

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

Many crashes. Will you try to run & test this branch @ColonelMoutarde ?

app/Models/Entry.php Outdated Show resolved Hide resolved
@Alkarex
Copy link
Member

Alkarex commented Nov 16, 2023

Please let me know when you are done testing & fixing @ColonelMoutarde

@ColonelMoutarde
Copy link
Contributor Author

Please let me know when you are done testing & fixing @ColonelMoutarde

Ok for me ;)

@Alkarex
Copy link
Member

Alkarex commented Nov 16, 2023

Are you able to run this branch for all the main functionalities?

Just loading the main page:

Fatal error: Uncaught TypeError: Minz_PdoSqlite::__construct():
Argument #2 ($username) must be of type ?string, bool given,
called in /var/www/FreshRSS/lib/Minz/ModelPdo.php on line 61
and defined in /var/www/FreshRSS/lib/Minz/PdoSqlite.php:11
Stack trace:
#0 /var/www/FreshRSS/lib/Minz/ModelPdo.php(61): Minz_PdoSqlite->__construct()
#1 /var/www/FreshRSS/lib/Minz/ModelPdo.php(115): Minz_ModelPdo->dbConnect()
#2 /var/www/FreshRSS/app/Models/Factory.php(19): Minz_ModelPdo->__construct()
#3 /var/www/FreshRSS/app/Models/Context.php(153): FreshRSS_Factory::createCategoryDao()
#4 /var/www/FreshRSS/app/Controllers/indexController.php(39): FreshRSS_Context::updateUsingRequest()
#5 /var/www/FreshRSS/lib/Minz/Dispatcher.php(120): FreshRSS_index_Controller->normalAction()
#6 /var/www/FreshRSS/lib/Minz/Dispatcher.php(49): Minz_Dispatcher->launchAction()
#7 /var/www/FreshRSS/lib/Minz/FrontController.php(61): Minz_Dispatcher->run()
#8 /var/www/FreshRSS/p/i/index.php(59): Minz_FrontController->run()
#9 {main} thrown in /var/www/FreshRSS/lib/Minz/PdoSqlite.php on line 11

And there are most likely plenty more

Alkarex added a commit that referenced this pull request Nov 17, 2023
@Alkarex
Copy link
Member

Alkarex commented Nov 17, 2023

Another regression fixed #5891

@mkorthof
Copy link

@Alwaysin: for CustomJS extension after changing to the new methods, indeed fixes the private method errors. e.g.

diff --git a/xExtension-CustomJS/extension.php b/xExtension-CustomJS/extension.php
index db76899..f30e441 100644
--- a/xExtension-CustomJS/extension.php
+++ b/xExtension-CustomJS/extension.php
@@ -4,7 +4,7 @@ class CustomJSExtension extends Minz_Extension {
        public function init() {
                $this->registerTranslates();

-               $current_user = Minz_Session::param('currentUser');
+               $current_user = Minz_Session::paramString('currentUser');
                $filename =  'script.' . $current_user . '.js';
                $filepath = join_path($this->getPath(), 'static', $filename);

@@ -16,7 +16,7 @@ class CustomJSExtension extends Minz_Extension {
        public function handleConfigureAction() {
                $this->registerTranslates();

-               $current_user = Minz_Session::param('currentUser');
+               $current_user = Minz_Session::paramString('currentUser');
                $filename =  'script.' . $current_user . '.js';
                $staticPath = join_path($this->getPath(), 'static');
                $filepath = join_path($staticPath, $filename);
@@ -28,7 +28,7 @@ class CustomJSExtension extends Minz_Extension {
                        $tmpPath = explode(EXTENSIONS_PATH . '/', $filepath);
                        $this->permission_problem = $tmpPath[1];
                } else if (Minz_Request::isPost()) {
-                       $js_rules = html_entity_decode(Minz_Request::param('js-rules', ''));
+                       $js_rules = html_entity_decode(Minz_Request::paramString('js-rules', ''));
                        file_put_contents($filepath, $js_rules);
                }

@Alkarex
Copy link
Member

Alkarex commented Nov 17, 2023

@mkorthof PR welcome 👍🏻

@@ -1,5 +1,6 @@
#!/usr/bin/env php
<?php
declare(strict_types=1);
Copy link
Member

Choose a reason for hiding this comment

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

Regression for PHP 7, fixed in #5893

@Alkarex
Copy link
Member

Alkarex commented Nov 18, 2023

Multiple PHP 7 crashes, fixed in #5893

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Dec 6, 2023
Regression from FreshRSS#5830
@Alkarex Alkarex mentioned this pull request Dec 6, 2023
@Alkarex
Copy link
Member

Alkarex commented Dec 6, 2023

Another regression with crash in production #5927

Alkarex added a commit that referenced this pull request Dec 6, 2023
Regression from #5830
Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Dec 6, 2023
fix FreshRSS#5928
`susbstr` might return false in PHP7
Regression from FreshRSS#5830
@Alkarex Alkarex mentioned this pull request Dec 6, 2023
@Alkarex
Copy link
Member

Alkarex commented Dec 6, 2023

Another crash #5929

Alkarex added a commit that referenced this pull request Dec 6, 2023
fix #5928
`susbstr` might return false in PHP7
Regression from #5830
@Alkarex
Copy link
Member

Alkarex commented Dec 10, 2023

#5934

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Dec 10, 2023
@Alkarex Alkarex mentioned this pull request Dec 10, 2023
Alkarex added a commit that referenced this pull request Dec 10, 2023
fix #5934
Regression from #5830
@ColonelMoutarde ColonelMoutarde deleted the feature/littles-optimisations-and-booleansInConditions branch December 21, 2023 17:10
Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Dec 26, 2023
@Alkarex Alkarex mentioned this pull request Dec 26, 2023
@Alkarex
Copy link
Member

Alkarex commented Dec 26, 2023

Regression #5978

Alkarex added a commit that referenced this pull request Dec 26, 2023
@Alkarex
Copy link
Member

Alkarex commented Jan 2, 2024

#6015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
System care Everything related to the system care
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants