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

Updates coding standard to something PSR-12-ish #7886

Merged
merged 7 commits into from
Nov 21, 2023

Conversation

Sesquipedalian
Copy link
Member

PSR-12 with tab indentation and closing PHP tags (for now, anyway), plus a selection of other nice cleanup options.

@Sesquipedalian Sesquipedalian added this to the 3.0 Alpha 1 milestone Nov 15, 2023
@Sesquipedalian Sesquipedalian marked this pull request as draft November 15, 2023 21:08
PSR-12 with tab indentation and closing PHP tags (for now, anyway), plus a selection of other nice cleanup options.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Comment on lines 112 to 113
$request = Db::$db->query('', '
$request = Db::$db->query(
'',
'
Copy link
Contributor

Choose a reason for hiding this comment

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

This one's funny. Even though it looks strange, it's consistent with other params on their own lines.

Copy link
Member

@jdarwood007 jdarwood007 Nov 16, 2023

Choose a reason for hiding this comment

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

Well its a bc breakage. But we could introduce a identifierQuery() and drop the first param off this one. Considering like 95% of our queries don't need this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well its a bc breakage. But we could introduce a identifierQuery() and drop the first param off this one. Considering like 95% of our queries don't need this.

That's a good idea. We'd just need to switch which method the backward compatibility entry in Utils::$smcFunc pointed to, and we'd be good to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm. Here's another option that would introduce fewer complications for backward compatibility.

		$request = Db::$db->query(
			'',
			'SELECT id_member, validation_code, member_name, real_name, email_address, is_activated, passwd, lngfile
			FROM {db_prefix}members' . (empty($_REQUEST['u']) ? '
			WHERE member_name = {string:email_address} OR email_address = {string:email_address}' : '
			WHERE id_member = {int:id_member}') . '
			LIMIT 1',
			[
				'id_member' => isset($_REQUEST['u']) ? (int) $_REQUEST['u'] : 0,
				'email_address' => $_POST['user'] ?? '',
			],
		);

Taking out the extra line break at the start of the query string makes it look a lot less weird in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Does help but still looks odd. If this is easier then go this way. Maybe we introduce that in 3.1 or another future release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well its a bc breakage

Not is not. Bots never do bc breaks unless specifically told to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe @jdarwood007 meant that his idea to make an identifierQuery() method would break backward compatibility.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>

# Conflicts:
#	Sources/Security.php
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Copy link
Member

@jdarwood007 jdarwood007 left a comment

Choose a reason for hiding this comment

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

I give it my approval as this is just code standards and we just want to align on something that is easier enough for all of us to ensure our code meets prior to sending up PRs

@Sesquipedalian Sesquipedalian marked this pull request as ready for review November 16, 2023 03:53
@jdarwood007
Copy link
Member

FYI, there are only 42 (nice) uses of the identifier

jeremy@Zeus:~/repos/sm/SMF3.0(release-3.0○) » grep -Rn "Db::\$db->query(\s*'\w"
./other/upgrade_2-1_postgresql.sql:64:	Db::$db->query('truncate_table', '
./other/upgrade_2-1_mysql.sql:99:	Db::$db->query('truncate_table', '
./other/upgrade.php:1956:		Db::$db->query('substring', '
./Sources/tasks/FetchSMFiles.php:80:			Db::$db->query('substring', '
./Sources/ServerSideIncludes.php:488:		$request = Db::$db->query('substring', '
./Sources/ServerSideIncludes.php:632:		$request = Db::$db->query('substring', '
./Sources/ServerSideIncludes.php:665:		$request = Db::$db->query('substring', '
./Sources/PersonalMessage/PM.php:1255:			$request = Db::$db->query('pm_find_username', '
./Sources/User.php:1399:				Db::$db->query('delete_log_online_interval', '
./Sources/Actions/Recent.php:192:		$request = Db::$db->query('substring', '
./Sources/Actions/Admin/Maintenance.php:275:		$result = Db::$db->query('order_by_board_order', '
./Sources/Actions/Admin/Maintenance.php:913:		Db::$db->query('truncate_table', '
./Sources/Actions/Admin/Maintenance.php:921:		Db::$db->query('truncate_table', '
./Sources/Actions/Admin/Maintenance.php:924:		Db::$db->query('truncate_table', '
./Sources/Actions/Admin/Maintenance.php:927:		Db::$db->query('truncate_table', '
./Sources/Actions/Admin/Subscriptions.php:739:				Db::$db->query('substring', '
./Sources/Actions/Admin/Bans.php:1977:			Db::$db->query('truncate_table', '
./Sources/Actions/Admin/Themes.php:350:			$request = Db::$db->query('themes_count', '
./Sources/Actions/Admin/Themes.php:464:					Db::$db->query('substring', '
./Sources/Actions/Admin/Themes.php:476:					Db::$db->query('substring', '
./Sources/Actions/Admin/Themes.php:527:					Db::$db->query('substring', '
./Sources/Actions/Admin/Themes.php:539:					Db::$db->query('substring', '
./Sources/Actions/Admin/Reports.php:292:		$request = Db::$db->query('order_by_board_order', '
./Sources/Actions/Admin/ErrorLog.php:580:			Db::$db->query('truncate_table', '
./Sources/Actions/Admin/Tasks.php:412:			Db::$db->query('truncate_table', '
./Sources/Actions/Admin/Boards.php:988:		$request = Db::$db->query('order_by_board_order', '
./Sources/Actions/Admin/Calendar.php:418:			$request = Db::$db->query('order_by_board_order', '
./Sources/Actions/Unread.php:840:		$this->topic_request = Db::$db->query('substring', '
./Sources/Actions/Unread.php:906:		$this->topic_request = Db::$db->query('substring', '
./Sources/Actions/Search.php:174:		$request = Db::$db->query('order_by_board_order', '
./Sources/Actions/UnreadReplies.php:267:		$this->topic_request = Db::$db->query('substring', '
./Sources/Actions/UnreadReplies.php:296:		$request = Db::$db->query('unread_fetch_topic_count', '
./Sources/Actions/UnreadReplies.php:364:		$this->topic_request = Db::$db->query('substring', '
./Sources/Actions/Profile/IgnoreBoards.php:74:		$request = Db::$db->query('order_by_board_order', '
./Sources/Actions/Profile/StatPanel.php:103:		$result = Db::$db->query('distinct_poll_votes', '
./Sources/Actions/Profile/StatPanel.php:155:		$result = Db::$db->query('profile_board_stats', '
./Sources/Actions/Profile/StatPanel.php:188:		$result = Db::$db->query('user_activity_by_time', '
./Sources/Actions/Profile/ShowPermissions.php:88:		$request = Db::$db->query('order_by_board_order', '
./Sources/Actions/Credits.php:371:			$request = Db::$db->query('substring', '
./Sources/Actions/MessageIndex.php:241:		$request = Db::$db->query('order_by_board_order', '
./Sources/Actions/MessageIndex.php:668:		$result = Db::$db->query('substring', '
./Sources/Actions/Memberlist.php:324:			$request = Db::$db->query('substring', '

Copy link
Contributor

Choose a reason for hiding this comment

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

To me, blank line before "break" looks weird. But if that is in PSR-12 let's go with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not required by PSR-12. I've tweaked the rules now to change that bit.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian Sesquipedalian merged commit b32ffd6 into SimpleMachines:release-3.0 Nov 21, 2023
3 checks passed
@Sesquipedalian Sesquipedalian deleted the psr-12-ish branch November 21, 2023 23:05
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.

4 participants