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

[BUGFIX] Parse build task class correctly #344

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

Conversation

coldblooded01
Copy link

No description provided.

@coldblooded01 coldblooded01 changed the base branch from 4 to 4.6.1 April 13, 2021 00:03
@coldblooded01 coldblooded01 changed the base branch from 4.6.1 to 4.6 April 13, 2021 02:11
@coldblooded01 coldblooded01 changed the base branch from 4.6 to 4.7 July 14, 2021 22:08
Copy link
Collaborator

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Could you please provide a description of what you're trying to fix here

@michalkleiner
Copy link
Collaborator

Hi @emteknetnz, the OP doesn't have access to the repo anymore. The issue we had was that the check was failing for some task classes. Looking at the suggested fix, it was either there were some white characters around the actual class name or it was checking against itself when the BuildTask class was injected over.

The suggested change is quite safe from my perspective. I could potentially dig up the exact reason but not sure how beneficial that would be anyway.

@emteknetnz
Copy link
Collaborator

I don't think is_a is better than is_subclass_of here, because BuildTask is an abstract class. This shouldn't be getting injected over injected over.

Seems like all this needs is a trim() added? ... though I cannot think of why there would ever be additional space characters around $this->TaskClass

@michalkleiner
Copy link
Collaborator

You're probably right. We ended deploying the fork with this change on a single site so I'll look into why it needed this fix and if the trim was enough (and/or if it should be fixed somewhere else).

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

Successfully merging this pull request may close these issues.

3 participants