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

feat: change scandir by symfony finder #197

Merged
merged 2 commits into from Mar 30, 2023

Conversation

fox-john
Copy link
Contributor

Questions Answers
Description? remove all scandir call and use symfony Finder
Type? improvement
BC breaks? no
Deprecations? no
How to test? Test install / uninstall of module and update to 2.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Several things here. See comments in the code

    //use Symfony\Component\Finder\Finder;
    $finder = (new Finder())
        ->files()
        ->in(dirname(__DIR__) . '/psgdpr/sql/install')
        ->name('*.sql')//We deal only with sql files
    ;

    //This is needed because if $finder->hasResults() return true but the files are empty there will be no query executions
    //and then we should return false
    $hasExecutedAtLeastOneQuery = false;
    //use Symfony\Component\Finder\SplFileInfo
    /** @var SplFileInfo $file */
    foreach ($finder as $file) {
        $contents = $file->getContents();
        //Test first avoid using str_replace needlessly
        if ($contents === '') {
            continue;
        }

        $hasExecutedAtLeastOneQuery = true;
        $query = str_replace('PREFIX_', _DB_PREFIX_, $contents);

        \Db::getInstance()->execute($query);
    }

    return $hasExecutedAtLeastOneQuery;

You can remove the comments

Copy link
Contributor

Choose a reason for hiding this comment

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

About executeQuerySql method several things. See comments in the code

/** @var EntityManager $entityManager */
        $entityManager = $this->get('doctrine.orm.entity_manager');

        $finder = (new Finder())
            ->files()
            ->in(dirname(__DIR__) . '/psgdpr/sql/' . $folder)
            ->name('*.sql')//We deal only with sql files
        ;

        //This is needed because if $finder->hasResults() return true but the files are empty there will be no query executions
        //and then we should return false
        $hasExecutedAtLeastOneQuery = false;
        /** @var SplFileInfo $file */
        foreach ($finder as $file) {
            $contents = $file->getContents();
            //Test first avoid using str_replace needlessly
            if ($contents === '') {
                continue;
            }

            $hasExecutedAtLeastOneQuery = true;
            $query = str_replace('PREFIX_', _DB_PREFIX_, $contents);


            $entityManager->getConnection()->executeQuery($query);
        }

        $entityManager->flush();//Didn't it work without this line before?

        return $hasExecutedAtLeastOneQuery;

You can remove the comments

psgdpr.php Outdated
Comment on lines 199 to 212
public function enable($force_all = false)
{
$this->executeQuerySql(self::SQL_QUERY_TYPE_INSTALL);

return parent::enable($force_all);
}

public function disable($force_all = false)
{
$this->executeQuerySql(self::SQL_QUERY_TYPE_UNINSTALL);

return parent::disable($force_all);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what are they for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an oversight, I had put that for my tests. i remove it.

@PrestaPierre
Copy link

Hello !
Good for QA

@paulnoelcholot
Copy link

Hello @fox-john,

I have tested your PR and it's good for me! 🎉

Thanks!

@Progi1984 Progi1984 added this to the 2.0.0 milestone Mar 29, 2023
@nicosomb nicosomb merged commit 6744c90 into PrestaShop:dev Mar 30, 2023
6 checks passed
@nicosomb
Copy link
Contributor

Thank you @fox-john

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants