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

Master branch up to php 7 compatibility #107

Merged
merged 2 commits into from Jan 12, 2017

Conversation

diolektor
Copy link
Collaborator

@diolektor diolektor commented Jan 11, 2017

Поддержка php 7 в master-бранче для обеспечения работоспособности движка на свежих конфигурациях.

@Exileum Exileum changed the title php 7 future Master branch up to php 7 compatibility Jan 11, 2017
@Exileum Exileum self-requested a review January 11, 2017 22:35
@Exileum Exileum self-assigned this Jan 11, 2017
@Exileum Exileum added this to the Версия 2.1.6 milestone Jan 11, 2017
@@ -304,7 +304,7 @@ function make_rand_str ($len = 10)
$str = '';
while (strlen($str) < $len)
{
$str .= str_shuffle(preg_replace('#[^0-9a-zA-Z]#', '', crypt(uniqid(mt_rand(), true))));
$str .= str_shuffle(preg_replace('#[^0-9a-zA-Z]#', '', crypt(uniqid(mt_rand(), true), 'salt')));
Copy link
Member

Choose a reason for hiding this comment

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

Может заменим на более современную функцию password_hash(uniqid(mt_rand(), true), PASSWORD_BCRYPT), как в develop делали?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

сделаю черепик.

//
Copy link
Member

Choose a reason for hiding this comment

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

Вообще это сторонняя библиотека, но да ладно, я так посмотрел там вообще обновлений не было 4 года.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

по хорошему нужно переделать на подключение через бд.

@@ -606,7 +606,7 @@ function bbcode2html ($text)
/**
* Clean up
*/
static function clean_up ($text)
static public function clean_up ($text)
Copy link
Member

Choose a reason for hiding this comment

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

Нарушает PSR-2, должно быть public static, вот из документации:

Visibility MUST be declared on all properties and methods; abstract and final MUST be declared before the visibility; static MUST be declared after the visibility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

$this->debug('start');

$connect_type = ($this->cfg['persist']) ? 'mysql_pconnect' : 'mysql_connect';
$this->link = mysqli_connect($this->cfg['dbhost'], $this->cfg['dbuser'], $this->cfg['dbpasswd'], $this->cfg['dbname']);
Copy link
Member

Choose a reason for hiding this comment

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

С учетом что пропал параметр persist, может с конфига тоже выкинем?

Copy link
Collaborator

@leroy0 leroy0 Jan 12, 2017

Choose a reason for hiding this comment

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

Этот код неправильный - persist нельзя выкидывать, а то будет деградация. Вот так надо:
$this->link = mysqli_connect($this->cfg['persist'] ? "p:".$this->cfg['dbhost'] : $this->cfg['dbhost'], $this->cfg['dbuser'], $this->cfg['dbpasswd'], $this->cfg['dbname'])

{
return is_resource($this->link) ? mysql_affected_rows($this->link) : -1;
return $this->link instanceof mysqli ? mysqli_affected_rows($this->link) : 0;
Copy link
Member

Choose a reason for hiding this comment

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

Меня волнует с какой целью возвращали отрицательное значение раньше если ничего не было? Как бы не оказалось очередным костылем. Но в целом, ок.

Copy link
Collaborator

@leroy0 leroy0 Jan 12, 2017

Choose a reason for hiding this comment

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

Код неправильный, ломает ненужный метод query_info, также mysqli_affected_rows может возращать -1 в случае неудачного запроса. Вообще ненужную проверку $this->link instanceof mysqli можно смело заменить на isset($this->link) и если совсем уж паранойя, то заприватить link, он может быть либо null, либо mysqli и ничем другим. Здесь же код надо написать так: return isset($this->link) ? mysqli_affected_rows($this->link) : null;, потому что mysqli_affected_rows возращает null в случае передачи говна в аргументе, так что тут получается проверка для того, чтобы не получить warning.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Я погорячился насчёт query_info, не ломает он его. Но возращать 0 нельзя - это индикатор удачно выполненного запроса.

@@ -244,12 +219,19 @@ function sql_fetchfield($field, $rownum = -1, $query_id = 0)
}
}

private function sql_result(mysqli_result $res, $row, $field = 0)
Copy link
Member

Choose a reason for hiding this comment

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

Что-то тут с форматированием не так в сравнении с остальным файлом по отступам, переформат в PSR-2 будет, но все же 😀

{
if ($result OR $result = $this->result)
{
$return_value = is_resource($result) ? mysql_free_result($result) : false;
if ($result instanceof mysqli_result) {
Copy link
Member

Choose a reason for hiding this comment

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

А если нет? Раньше возвращало false, ничего не отломалось?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ничего не отломается, все вызовы sql_freeresult как void.

}

$message = str_replace('\"', '"', substr(@preg_replace('#(\>(((?>([^><]+|(?R)))*)\<))#se', "@preg_replace(\$orig_word, \$replacement_word, '\\0')", '>' . $message . '<'), 1, -1));
$message = str_replace(
Copy link
Member

Choose a reason for hiding this comment

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

Это круто прям 👍

Copy link
Member

@Exileum Exileum left a comment

Choose a reason for hiding this comment

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

В принципе по мелочам поправить прокомментированное и можно сливать. Большое спасибо.

Copy link
Member

@Exileum Exileum left a comment

Choose a reason for hiding this comment

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

Мелочь с public static и все ок 👌

@Exileum Exileum merged commit a04df19 into torrentpier:master Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants