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

IBX-394: Added lazy to registry cronjobs service #21

Merged
merged 1 commit into from
May 21, 2021

Conversation

mateuszdebinski
Copy link
Contributor

Question Answer
JIRA issue IBX-394
Type bug
Target Ibexa version v3.3
BC breaks no
Doc needed no

After changed ConsoleCommandListener subscription from MVCEvents::CONSOLE_INIT to ConsoleEvents::COMMAND CronJobsRegistry is not take siteaccess from ConsoleCommandListener

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Checked that target branch is set correctly.

Copy link

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

In general I'd personally prefer to have a "Registry" service instead, that I'd be able to call getCurrentSiteAccess() on, but that's outside the scope of this PR.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Are you sure 3.0 is still supported? AFAICS Ibexa 3.2 has already 3.1 version of that package. Double check please.

In general I'd personally prefer to have a "Registry" service instead, that I'd be able to call getCurrentSiteAccess() on, but that's outside the scope of this PR.

Actually... I see this is for 3.2. Could you check if \eZ\Publish\Core\MVC\Symfony\SiteAccess\SiteAccessServiceInterface::getCurrent works for you?

@Steveb-p
Copy link

Could you check if \eZ\Publish\Core\MVC\Symfony\SiteAccess\SiteAccessServiceInterface::getCurrent works for you?

If this service could be used instead of marking the console command as lazy, that would be my go to solution. Lazy services are a workaround in this case for using a property that changes on runtime, and it does not work 100% of the time since it can change more than once, after initialization of the proxy.

@mateuszdebinski mateuszdebinski changed the base branch from 3.0 to 3.1 May 20, 2021 10:51
@mateuszdebinski
Copy link
Contributor Author

I changed the branch to 3.1 because IBEXA 3.2 works fine only in 3.3 is a problem. I will add this service to CronJobsRegistry and check

@mateuszdebinski
Copy link
Contributor Author

\eZ\Publish\Core\MVC\Symfony\SiteAccess\SiteAccessServiceInterface::getCurrent not working always return "default" like now

@Steveb-p
Copy link

\eZ\Publish\Core\MVC\Symfony\SiteAccess\SiteAccessServiceInterface::getCurrent not working always return "default" like now

That's kinda bad. It should update just like the container updates 😕

@alongosz
Copy link
Member

\eZ\Publish\Core\MVC\Symfony\SiteAccess\SiteAccessServiceInterface::getCurrent not working always return "default" like now

That's kinda bad. It should update just like the container updates

@Steveb-p after further debugging I see the method which requires SA name is used when building a Container via addMethodCall. To fix that, every cron-executed command would need to fetch the information on its own, which is not possible. Let me know if you think I'm missing something.

@Steveb-p
Copy link

To fix that, every cron-executed command would need to fetch the information on its own, which is not possible. Let me know if you think I'm missing something.

I have no idea how specifically this package works, so if it's a bugfix and other than that it works as designed, then I don't mind it going like this.

@bogusez bogusez self-assigned this May 21, 2021
@lserwatka lserwatka merged commit 4a222b1 into 3.1 May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants