Skip to content

fix: add check for duplicate Registrar Auto-Discovery runs #9073

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

Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 25, 2024

Description
See #9069

Any Registrar class should be a class that simply returns an array without side effects.
If a Registrar file instantiates any Config class, it runs Auto-Discovery again during Auto-Discovery, and duplicate runs are not expected.

This PR checks for duplicate Registrar Auto-Discovery runs, and throws Exception.

How to Test

<?php

namespace Config;

class Registrar
{
    public static function hack(): void
    {
        $config = config('Cache');

        // Does something.
    }
}

Registrar::hack(); // Bad. When this class is loaded, Config\Cache will be instantiated.
$ ./spark config:check App
PHP Fatal error:  Uncaught CodeIgniter\Exceptions\ConfigException: During Auto-Discovery of Registrars, "Config\Cache" executes Auto-Discovery again. "APPPATH/Config/Registrar.php" seems to have bad code. in /.../CodeIgniter4/system/Config/BaseConfig.php:246
Stack trace:
...

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis force-pushed the check-for-duplicate-registrar-discovery branch from 0b31522 to f609bea Compare July 25, 2024 04:41
@kenjis kenjis changed the title fix: add check for duplicate registrar auto-discovery runs fix: add check for duplicate Registrar Auto-Discovery runs Jul 25, 2024
@kenjis kenjis added breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them labels Jul 25, 2024
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

I think this change makes sense. Just waiting for a changelog to approve.

@kenjis kenjis force-pushed the check-for-duplicate-registrar-discovery branch 2 times, most recently from fbb3177 to 3144e1c Compare July 26, 2024 01:25
@kenjis
Copy link
Member Author

kenjis commented Jul 26, 2024

Added the docs.

@kenjis
Copy link
Member Author

kenjis commented Jul 26, 2024

Is it better to send to 4.6 branch?

@neznaika0
Copy link
Contributor

Maybe not, we don't know how many people use registrar, I think it's not very critical to have duplicates. The longer we wait for 4.6, the more we need to update

@michalsn
Copy link
Member

Oops... I thought this change was for the 4.6 branch. Well, IMO we should make this change in 4.6.

Bonfire is probably relatively popular, so if we "fix" this problem in a patch release, we will make all applications that rely on it broken. Targeting v4.6 will give time for the project to work out a fix.

There may be other projects that use a similar hack. Minor releases in the CI4 world have made everyone accustomed to carefully reviewing the change log or upgrade notes.

@kenjis kenjis force-pushed the check-for-duplicate-registrar-discovery branch from 3144e1c to 0956afd Compare July 26, 2024 06:52
@kenjis kenjis changed the base branch from develop to 4.6 July 26, 2024 06:52
@kenjis kenjis added the 4.6 label Jul 26, 2024
@kenjis
Copy link
Member Author

kenjis commented Jul 26, 2024

Okay. Changed target to 4.6.

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Thanks!

@kenjis kenjis force-pushed the check-for-duplicate-registrar-discovery branch from 0956afd to eb53f86 Compare July 28, 2024 00:40
@kenjis kenjis merged commit 2d8b3bd into codeigniter4:4.6 Jul 29, 2024
42 checks passed
@kenjis kenjis deleted the check-for-duplicate-registrar-discovery branch July 29, 2024 21:31
@kenjis
Copy link
Member Author

kenjis commented Jul 29, 2024

Thank you all for your reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants