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

Do not initialize in tasks the Core by default, only when requested #1160

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Quetzacoalt91
Copy link
Member

@Quetzacoalt91 Quetzacoalt91 commented Feb 5, 2025

Questions Answers
Description? We had a weird implementation of a default initialization of the core, that would suppressed by each task if necessary. We should actually rely on the core ONLY when necessary, so only the task needing it to work will call the method initPrestaShopCore(). This PR also moves a XML initialization to a specific place instead of at the beginning of each update task.
Type? bug fix + improvement
BC breaks? Nope
Deprecations? Nope
Fixed ticket? /
Sponsor company /
How to test? Backup, Updates and Restore are still possible, especially when using the local archive.

@Quetzacoalt91 Quetzacoalt91 added bug Type: Bug fix enhancement Type: Improvement labels Feb 5, 2025
@Quetzacoalt91 Quetzacoalt91 self-assigned this Feb 5, 2025
ga-devfront
ga-devfront previously approved these changes Feb 6, 2025
$this->container->getFileLoader()->addXmlMd5File($this->container->getUpgrader()->getDestinationVersion(), $this->container->getProperty(UpgradeContainer::DOWNLOAD_PATH) . DIRECTORY_SEPARATOR . $archiveXml);
}
}
public function init(): void {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can change this to

Suggested change
public function init(): void {}
abstract public function init(): void;

Copy link
Member Author

Choose a reason for hiding this comment

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

By declaring the method as abstract, I will need to define it on each task even if I have nothing to call. This is not the behavior I wanted.

Comment on lines 199 to 204
$archiveXml = $this->container->getUpdateConfiguration()->getLocalChannelXml();
$this->container->getFileLoader()->addXmlMd5File(
$this->container->getUpgrader()->getDestinationVersion(),
$this->container->getProperty(UpgradeContainer::DOWNLOAD_PATH) . DIRECTORY_SEPARATOR . $archiveXml
);

Copy link
Contributor

Choose a reason for hiding this comment

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

this is not only necessary for the local channel

Suggested change
$archiveXml = $this->container->getUpdateConfiguration()->getLocalChannelXml();
$this->container->getFileLoader()->addXmlMd5File(
$this->container->getUpgrader()->getDestinationVersion(),
$this->container->getProperty(UpgradeContainer::DOWNLOAD_PATH) . DIRECTORY_SEPARATOR . $archiveXml
);
if ($this->container->getUpdateConfiguration()->isChannelLocal()) {
$archiveXml = $this->container->getUpdateConfiguration()->getLocalChannelXml();
$this->container->getFileLoader()->addXmlMd5File($this->container->getUpgrader()->getDestinationVersion(), $this->container->getProperty(UpgradeContainer::DOWNLOAD_PATH) . DIRECTORY_SEPARATOR . $archiveXml);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to commit it, indeed

@Quetzacoalt91 Quetzacoalt91 force-pushed the remove-default-initialization-of-core branch from 30b90e9 to 1c7ee5d Compare February 6, 2025 09:05
Copy link

sonarqubecloud bot commented Feb 6, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Type: Bug fix enhancement Type: Improvement waiting for QA
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

5 participants