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

check mysql using PDO now #487

Merged
merged 6 commits into from Aug 23, 2019

Conversation

@fiammybe
Copy link
Member

fiammybe commented Aug 14, 2019

fix #475

fix #475
@fiammybe fiammybe added the bug label Aug 14, 2019
@fiammybe fiammybe added this to the 1.4.0 milestone Aug 14, 2019
@fiammybe fiammybe self-assigned this Aug 14, 2019
@fiammybe fiammybe added this to In progress in v1.4.x via automation Aug 14, 2019
@fiammybe fiammybe requested review from MekDrop and skenow Aug 14, 2019
Copy link
Contributor

skenow left a comment

I see that you check for the PDO mysql driver availability, which is what it needs to be for PHP7. Does it look like the images displayed are always going to be success, except for the PHP version, and maybe not even then?

@fiammybe

This comment has been minimized.

Copy link
Member Author

fiammybe commented Aug 15, 2019

As a matter of fact, you are correct! I noticed the issue because of an incongruity between the checkbox and the status, and with this change the 2 are aligned again : both ok. But you are right that the image won't ever change with this code... I need to improve on that

Copy link

accesslint bot left a comment

There are accessibility issues in these changes.

htdocs/install/page_modcheck.php Outdated Show resolved Hide resolved
htdocs/install/page_modcheck.php Outdated Show resolved Hide resolved
htdocs/install/page_modcheck.php Outdated Show resolved Hide resolved
htdocs/install/page_modcheck.php Outdated Show resolved Hide resolved
@skenow

This comment has been minimized.

Copy link
Contributor

skenow commented Aug 16, 2019

It looks like the image selection is reversed - 'no' will display if the test is successful, 'yes' if it is not. The alt text could also be changed based on the result.

@fiammybe

This comment has been minimized.

Copy link
Member Author

fiammybe commented Aug 17, 2019

You're totally correct. And I also missed some cases. I'll try to make this a bit more structured before committing again. a function here and there wouldn't hurt :-)

The requirements are now defined in an array in imCheckRequirements, and the page just loops through the array to show the results.
Copy link

accesslint bot left a comment

There are accessibility issues in these changes.

htdocs/install/page_modcheck.php Outdated Show resolved Hide resolved
Copy link
Member

MekDrop left a comment

nice changes. few things to be good to fix

htdocs/install/page_modcheck.php Outdated Show resolved Hide resolved
htdocs/install/page_modcheck.php Outdated Show resolved Hide resolved
htdocs/install/page_modcheck.php Show resolved Hide resolved
Copy link

accesslint bot left a comment

There are accessibility issues in these changes.

htdocs/install/page_modcheck.php Outdated Show resolved Hide resolved
@fiammybe fiammybe requested review from MekDrop and skenow Aug 20, 2019
Copy link
Contributor

skenow left a comment

Logic looks OK - the only question is the double '?' on line 82

htdocs/install/page_modcheck.php Show resolved Hide resolved
@fiammybe fiammybe requested a review from skenow Aug 23, 2019
@skenow
skenow approved these changes Aug 23, 2019
Copy link
Contributor

skenow left a comment

All good!

@fiammybe fiammybe merged commit 305dd84 into branches/impresscms_1.4 Aug 23, 2019
2 of 3 checks passed
2 of 3 checks passed
AccessLint Reviewing for accessibility issues
WhiteSource Security Check Security Report
Details
WIP Ready for review
Details
v1.4.x automation moved this from In progress to Done Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v1.4.x
  
Done
3 participants
You can’t perform that action at this time.