adminalias.php/adminuser.php/site.php: $row['enabled']==="0" doesn't work for me, but $row['enabled']=="0" work fine (and other small fixes)

VVD commented 8 months ago

PHP 8.1, MariaDB 10.11.5, FreeBSD 13.2 amd64.

https://github.com/vexim/vexim2/blob/master/vexim/adminalias.php#L71 This patch fixed the issue:

-            if($row['enabled']==="0") print '<tr class="disabled">'; else print '<tr>';
+            if($row['enabled']=="0") print '<tr class="disabled">'; else print '<tr>';

https://github.com/vexim/vexim2/blob/master/vexim/adminuser.php#L91

-          if($row['enabled']==="0") print '<tr class="disabled">'; else print '<tr>';
+          if($row['enabled']=="0") print '<tr class="disabled">'; else print '<tr>';

https://github.com/vexim/vexim2/blob/master/vexim/site.php#L69

-          if($row['enabled']==="0") print '<tr class="disabled">'; else print '<tr>';
+          if($row['enabled']=="0") print '<tr class="disabled">'; else print '<tr>';

Also:

https://github.com/vexim/vexim2/blob/master/vexim/adminalias.php#L16
```
<a href="adminaliasadd.php"><?php echo _('Add Alias'); ?></a>
```
https://github.com/vexim/vexim2/blob/master/vexim/adminalias.php#L24
```
. '</a>';
```
 ? Maybe   or just  ?

https://github.com/vexim/vexim2/blob/master/vexim/admingroup.php Fixed incorrect show disabled groups:

@@ -33,7 +32,11 @@
       $sth->execute(array(':domain_id'=>$_SESSION['domain_id']));
       while ($row = $sth->fetch()) {
     ?>
+        <?php if ('1' == $row['enabled']) { ?>
     <tr>
+        <?php } else { ?>
+        <tr class="disabled">
+        <?php } ?>
       <td class="trash">
         <a href="admingroupdelete.php?group_id=<?php echo $row['id']; ?>&localpart=<?php echo $row['name']; ?>">
         <img class='trash' title="<?php print _('Delete group') . $row['name']; ?>"

https://github.com/vexim/vexim2/blob/master/vexim/admingroupchange.php#L45 Need extra space char:

-              value="<?php echo $row['name']; ?>"class="textfield" autofocus>@
+              value="<?php echo $row['name']; ?>" class="textfield" autofocus>@

https://github.com/vexim/vexim2/blob/master/vexim/admingroupchange.php#L69

-            <input name="editgroup" type="submit" value="Submit">
+            <input name="editgroup" type="submit" value="<?php echo _('Submit'); ?>">

https://github.com/vexim/vexim2/blob/master/vexim/admingroupaddsubmit.php#L22 Check for execute SQL errors:

@@ -17,9 +17,13 @@
 header("Location: admingroup.php?badname={$_POST['usertoadd']}");
 die;
}
$query = "INSERT INTO group_contents (group_id, member_id) VALUES (:group_id, :usertoadd)";
$sth = $dbh->prepare($query);
-  $success = $sth->execute(array(':group_id'=>$_POST['group_id'], ':usertoadd'=>$_POST['usertoadd']));
+  try {
+    $success = $sth->execute(array(':group_id'=>$_POST['group_id'], ':usertoadd'=>$_POST['usertoadd']));
+  } catch (PDOException $e) {
+    $success = 0;
+  }
if ($success) {
 header ("Location: admingroupchange.php?group_id={$_POST['group_id']}&group_updated={$_POST['localpart']}");
} else {

https://github.com/vexim/vexim2/blob/master/vexim/locale/ru/LC_MESSAGES/ru.po#L374 Fixed translation:
```
-msgstr "Добавить члена"
+msgstr "Добавить участника"
```

https://github.com/vexim/vexim2/blob/master/vexim/admingroupchange.php#L115 Align picture:

-                  <img class="check" src="images/check.gif">
+                  <div align="center"><img class="check" src="images/check.gif"></div>

P.S. I have patch for "Writer" in group - if you want I create other issue with patch.

rimas-kudelis commented 8 months ago

- if($row['enabled']==="0") print '<tr class="disabled">'; else print '<tr>';
+ if($row['enabled']=="0") print '<tr class="disabled">'; else print '<tr>';
Strict comparison is generally a good thing, but I guess we were supposed to compare values to integers and not strings in this case. On the other hand, for this old piece of code, == seems acceptable too.

 ? Maybe   or just  ?

These are definitely a mistake, you're right.

@@ -33,7 +32,11 @@
           $sth->execute(array(':domain_id'=>$_SESSION['domain_id']));
           while ($row = $sth->fetch()) {
         ?>
+        <?php if ('1' == $row['enabled']) { ?>
         <tr>
+        <?php } else { ?>
+        <tr class="disabled">
+        <?php } ?>
           <td class="trash">
             <a href="admingroupdelete.php?group_id=<?php echo $row['id']; ?>&localpart=<?php echo $row['name']; ?>">
             <img class='trash' title="<?php print _('Delete group') . $row['name']; ?>"

Perhaps it would be better to mirror the code style above, even if that style isn't very beautiful? At least line-wise it's more compact.

...

+1 on everything else

P.S. I have patch for "Writer" in group - if you want I create other issue with patch.

Please do.

VVD commented 7 months ago

Perhaps it would be better to mirror the code style above, even if that style isn't very beautiful? At least line-wise it's more compact.
https://github.com/vexim/vexim2/blob/master/vexim/admingroup.php
@@ -32,7 +32,7 @@
$sth->execute(array(':domain_id'=>$_SESSION['domain_id']));
while ($row = $sth->fetch()) {
+ if($row['enabled']=="0") print '<tr class="disabled">'; else print '<tr>';
?>
- <tr>
<td class="trash">
<a href="admingroupdelete.php?group_id=<?php echo $row['id']; ?>&localpart=<?php echo $row['name']; ?>">
<img class='trash' title="<?php print _('Delete group') . $row['name']; ?>"
Line if($row['enabled']=="0") print '<tr class="disabled">'; else print '<tr>'; copied from https://github.com/vexim/vexim2/blob/master/vexim/adminuser.php#L92 with fix === => ==. The patch tested - work for me.

Will you commit these patches?

P.S. I have patch for "Writer" in group - if you want I create other issue with patch.

Please do.

Ok. Need time to separate it from other patches.

VVD commented 7 months ago

https://github.com/vexim/vexim2/issues/281

VVD commented 7 months ago

Also patch for better align, IMHO (center or add extra  ):

diff -ur vexim.orig/admin.php vexim/admin.php
--- vexim.orig/admin.php
+++ vexim/admin.php
@@ -20,7 +20,7 @@
               <?php
                 echo _('Add, delete and manage POP/IMAP accounts');
               ?>
-            </a>
+            </a><br><br>
           </td>
         </tr>
         <tr>
@@ -29,14 +29,14 @@
               <?php
                 echo _('Add, delete and manage aliases, forwards and a Catchall');
               ?>
-            </a>
+            </a><br><br>
           </td>
         </tr>
         <tr>
           <td>
             <a href="admingroup.php">
               <?php echo _('Add, delete and manage groups'); ?>
-            </a>
+            </a><br><br>
           </td>
         </tr>
         <tr>
diff -ur vexim.orig/adminalias.php vexim/adminalias.php
--- vexim.orig/adminalias.php
+++ vexim/adminalias.php
@@ -13,18 +13,18 @@
   <body>
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
-      <a href="adminaliasadd.php"><?php echo _('Add Alias'); ?></a></br>
+      <a href="adminaliasadd.php"><?php echo _('Add Alias'); ?></a><br>
       <?php $query = "SELECT user_id,realname,smtp,localpart FROM users
         WHERE domain_id=:domain_id AND type='catch'";
         $sth = $dbh->prepare($query);
         $sth->execute(array(':domain_id'=>$_SESSION['domain_id']));
         if (!$sth->rowCount()) {
-          print '<a href="admincatchalladd.php">'
+          print '<br><a href="admincatchalladd.php">'
             . _('Add Catchall')
-            . '</a></br>';
+            . '</a><br>';
         }
       ?>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Content">
diff -ur vexim.orig/adminaliasadd.php vexim/adminaliasadd.php
--- vexim.orig/adminaliasadd.php
+++ vexim/adminaliasadd.php
@@ -19,7 +19,7 @@
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
       <a href="adminalias.php"><?php echo _('Manage Aliases'); ?></a><br>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Forms">
diff -ur vexim.orig/admincatchalladd.php vexim/admincatchalladd.php
--- vexim.orig/admincatchalladd.php
+++ vexim/admincatchalladd.php
@@ -14,8 +14,8 @@
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
       <a href="adminalias.php"><?php echo _('Manage Aliases'); ?></a><br>
-      <a href="adminaliasadd.php"><?php echo _('Add Alias'); ?></a></br>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <br><a href="adminaliasadd.php"><?php echo _('Add Alias'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Forms">
diff -ur vexim.orig/adminfail.php vexim/adminfail.php
--- vexim.orig/adminfail.php
+++ vexim/adminfail.php
@@ -14,7 +14,7 @@
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
       <a href="adminfailadd.php"><?php echo _('Add Fail'); ?></a><br>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Content">
diff -ur vexim.orig/adminfailadd.php vexim/adminfailadd.php
--- vexim.orig/adminfailadd.php
+++ vexim/adminfailadd.php
@@ -14,7 +14,7 @@
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
        <a href="adminfail.php"><?php echo _('Manage Fails'); ?></a><br>
-       <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+       <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
        <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Forms">
diff -ur vexim.orig/admingroup.php vexim/admingroup.php
--- vexim.orig/admingroup.php
+++ vexim/admingroup.php
@@ -12,9 +12,8 @@
   <body>
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
-      <a href="admingroupadd.php"><?php echo _('Add Group'); ?></a>
-      <br>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <a href="admingroupadd.php"><?php echo _('Add Group'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Content">
@@ -46,16 +45,20 @@
             <?php echo $row['name'].'@'.$_SESSION['domain']; ?></a>
           </td>
           <td>
+            <div align="center">
             <?php if ('Y' == $row['is_public']) { ?>
               <img class="check" src="images/check.gif"
                 title="<?php print _('Anyone can write to') . ' '. $row['name']; ?>">
             <?php } ?>
+            </div>
           </td>
           <td>
+            <div align="center">
             <?php if ('1' == $row['enabled']) { ?>
               <img class="check" src="images/check.gif"
                 title="<?php print $row['name'] . _(' is enabled'); ?>">
             <?php } ?>
+            </div>
           </td>
         </tr>
         <?php
diff -ur vexim.orig/admingroupadd.php vexim/admingroupadd.php
--- vexim.orig/admingroupadd.php
+++ vexim/admingroupadd.php
@@ -13,7 +13,7 @@
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
       <a href="admingroup.php"><?php echo _('Manage Groups'); ?></a><br>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Forms">
diff -ur vexim.orig/admingroupchange.php vexim/admingroupchange.php
--- vexim.orig/admingroupchange.php
+++ vexim/admingroupchange.php
@@ -22,8 +22,8 @@
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
       <a href="admingroup.php"><?php echo _('Manage Groups'); ?></a><br>
-      <a href="admingroupadd.php"><?php echo _('Add Group'); ?></a></br>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <br><a href="admingroupadd.php"><?php echo _('Add Group'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Forms">
diff -ur vexim.orig/adminuser.php vexim/adminuser.php
--- vexim.orig/adminuser.php
+++ vexim/adminuser.php
@@ -42,7 +42,7 @@
         }
       ?>
       <br>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="Content">
diff -ur vexim.orig/adminuseradd.php vexim/adminuseradd.php
--- vexim.orig/adminuseradd.php
+++ vexim/adminuseradd.php
@@ -37,7 +37,7 @@
     <?php include dirname(__FILE__) . '/config/header.php'; ?>
     <div id="Menu">
       <a href="adminuser.php"><?php echo _('Manage Accounts'); ?></a><br>
-      <a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
+      <br><a href="admin.php"><?php echo _('Main Menu'); ?></a><br>
       <br><a href="logout.php"><?php echo _('Logout'); ?></a><br>
     </div>
     <div id="forms">

Udera commented 6 months ago

Strict comparison is generally a good thing, but I guess we were supposed to compare values to integers and not strings in this case. On the other hand, for this old piece of code, == seems acceptable too.

We have added it at a few parts, not sure here the database uses tinyint(1), so the value is probably an int. I´d rather change it in this sense. I think that I can perhaps do a PR with a few changes at least to make it work again.

rimas-kudelis commented 6 months ago

We have added it at a few parts, not sure here the database uses tinyint(1), so the value is probably an int. I´d rather change it in this sense. I think that I can perhaps do a PR with a few changes at least to make it work again.

I'm not sure what PR you're suggesting: one where === is changed to == or one where "1" is changed to 1? I think I'd vote for the former, not latter, although both are fine.

Udera commented 6 months ago

one where "1" is changed to 1

this one.

VVD commented 6 months ago

I found one more . Full list is:

admincatchalladd.php:      <a href="adminaliasadd.php"><?php echo _('Add Alias'); ?></a></br>
adminaliaschange.php:      <a href="adminaliasadd.php"><?php echo _('Add Alias'); ?></a></br>
adminalias.php:      <a href="adminaliasadd.php"><?php echo _('Add Alias'); ?></a></br>
adminalias.php:            . '</a></br>';
admingroupchange.php:      <a href="admingroupadd.php"><?php echo _('Add Group'); ?></a></br>

rimas-kudelis commented 6 months ago

Thanks, I've fixed the line breaks in 27fcc82.

rimas-kudelis commented 6 months ago

Highlighting of disabled users, aliases and sites fixed in eff3823.
Checkboxes centered in 934b086.
Russian l10n file updated in 1b4074f.

VVD commented 6 months ago

Thanks a lot!

Only 9th left from 1st comment. Is it in progress or something wrong with patch?

Maybe change this too vexim/admingroup.php (55):

-            <?php if ('1' == $row['enabled']) { ?>
+            <?php if (1 === $row['enabled']) { ?>

vexim/admingroupchange.php (113):

-                    if($row['enabled']=='1') {
+                    if(1 === $row['enabled']) {

I think we have a lot more of the "same" lines in code. If you want - I can search:

$ grep -R == vexim | wc -l
     151

:-D

rimas-kudelis commented 6 months ago

Thanks a lot!

Only 9th left from 1st comment. Is it in progress or something wrong with patch?

I just skipped that one. The only place where we handle \PDOExceptions is in variables.php where we're establishing a database connection. Nowhere else in the code are we handling them. Handling it in just this one extra place doesn't make sense to me.

Are you encountering that exception there specifically?

Maybe change this too vexim/admingroup.php (55):
- <?php if ('1' == $row['enabled']) { ?>
+ <?php if (1 === $row['enabled']) { ?>
vexim/admingroupchange.php (113):
- if($row['enabled']=='1') {
+ if(1 === $row['enabled']) {
I think we have a lot more of the "same" lines in code. If you want - I can search:
$ grep -R == vexim | wc -l
 151
:-D

These lines work well already due to non-strict matching.

As for all non-strict matches in general, it's like I said numerous times, this mess of intermixed PHP and HTML is old and smells, but I don't see much point in trying to improve it.

I believe a full rewrite would take less time than a global cleanup though, because most of the stuff can be generated automatically.

Btw, your grep line also matches === and !==. :wink:

VVD commented 6 months ago

Are you encountering that exception there specifically?

Yes. But I can't remember details. Maybe even incorrect user input… Maybe trying to add same user 2nd time… BTW, correct file is: vexim/admingroupcontentaddsubmit.php.

These lines work well already due to non-strict matching. As for all non-strict matches in general, it's like I said numerous times, this mess of intermixed PHP and HTML is old and smells, but I don't see much point in trying to improve it. I believe a full rewrite would take less time than a global cleanup though, because most of the stuff can be generated automatically.

Ok. Agree.

Btw, your grep line also matches === and !==. 😉

I know. :-P 151 lines ez to filter manually.

vexim / vexim2

adminalias.php/adminuser.php/site.php: $row['enabled']==="0" doesn't work for me, but $row['enabled']=="0" work fine (and other small fixes) #279