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

[5.3] Deprecations: Changes we can make with 5.3 for the actionlog component (Backend) #44261

Open
wants to merge 18 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

rdeutz
Copy link
Contributor

@rdeutz rdeutz commented Oct 15, 2024

Summary of Changes

Implementing changes to our codebase regarding depreciated code. Done by component and splited into back and frontend for easier testing.

Testing Instructions

Test the backend for the action log component. Test also other componets e.g. user who add information to the action log.

Actual result BEFORE applying this Pull Request

Anything works

Expected result AFTER applying this Pull Request

Anything works

@rdeutz rdeutz changed the title [5.2] Deprecations: Changes we can make with 5.3 [5.3] Deprecations: Changes we can make with 5.3 Oct 15, 2024
@brianteeman
Copy link
Contributor

Can you please update the PR title so that it is clearer what is being changed here.

@rdeutz rdeutz changed the title [5.3] Deprecations: Changes we can make with 5.3 [5.3] Deprecations: Changes we can make with 5.3 for the actionlog component (Backend) Oct 15, 2024
@HLeithner
Copy link
Member

@brianteeman what's the issue when removing (C) ? the @copyright already says that it's about the copyright isn't it?
as alternative we could use © can't we? just asking

@brianteeman
Copy link
Contributor

@brianteeman what's the issue when removing (C) ? the @copyright already says that it's about the copyright isn't it? as alternative we could use © can't we? just asking

  1. We should only use one form of copyright statement throughout Joomla
  2. In many countries - including the USA - the @copyright "element" does not match the legal requirements of a copyright statement
  3. Using © instead of (c) is ok but we must be consistent

I can provide more legal references if required

@rdeutz
Copy link
Contributor Author

rdeutz commented Oct 17, 2024

@brianteeman using "Copyright" as word should also be ok, right? But I am looking what is the best way in terms of IDE configuration.

@brianteeman
Copy link
Contributor

@brianteeman using "Copyright" as word should also be ok, right? But I am looking what is the best way in terms of IDE configuration.

In the USA it would be ok - not sure about elsewhere. We should be consistent though.

@rdeutz
Copy link
Contributor Author

rdeutz commented Oct 17, 2024

I did it again on another way, so anything should be fine now and the PR can be tested

@brianteeman
Copy link
Contributor

I am confused about this codestyle as it doesnt appear to be consistent

        if (!empty($extension)) {
            $extension .= '%';
            $query
                ->where($db->quoteName('a.extension') . ' LIKE :extension')
                ->bind(':extension', $extension);
        }
                $query->where(
                    $db->quoteName('a.log_date') . ' BETWEEN :dstart AND :dnow',
                );
                $query->bind(':dstart', $dStart);
                $query->bind(':dnow', $dNow);

In the second one shouldnt the two bind queries be part of the first query - so something like

                $query
                ->where($db->quoteName('a.log_date') . ' BETWEEN :dstart AND :dnow')
                ->bind(':dstart', $dStart);
                ->bind(':dnow', $dNow);

@rdeutz
Copy link
Contributor Author

rdeutz commented Oct 17, 2024

I missed the whole file.

About the code style for multiline chaining. Honestly I am not 100% sure what it better readable. The reformat applies when you have something like

$var->action1()
  ->action2()
  ->action3()

then it is converted into

$var
  ->action1()
  ->action2()
  ->action3()

It might be better to let it as it is, the difference is not so big

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.

5 participants