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

docs: calling cells without a full namespace #8327

Closed
wants to merge 1 commit into from

Conversation

michalsn
Copy link
Member

Description
This PR adds a note about calling cells without a full namespace.

Fixes #8326

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
Copy link
Member

kenjis commented Dec 13, 2023

Is the current behavior intentional?
I found this comment:

// @TODO extend Factories to be able to load classes with the same short name.

@michalsn
Copy link
Member Author

michalsn commented Dec 14, 2023

My guess is it's intentional. Otherwise, we would call it like:

Factories::cells($class, ['getShared'  => false])

which should be enough to get a new instance every time.

Edit:
I have mixed feelings about the comment. Extending is not necessary.

@michalsn
Copy link
Member Author

Well... the more I think about it, the more I'm convinced the better option would be to use

Factories::cells($class, ['getShared'  => false])

If we call the same cell a couple of times, the odds we will use the same data is probably very rare.

Thoughts?

@kenjis
Copy link
Member

kenjis commented Dec 14, 2023

At least, I don't know why it creates a new instance when a FQCN comes but shares the instance when a shortname comes.

$object = class_exists($class) ? new $class() : Factories::cells($class);

@michalsn
Copy link
Member Author

Seems like it was introduced here: #7686
As a fix for calling the cell with the same name? Before this, we were using factories exclusively.

@kenjis
Copy link
Member

kenjis commented Dec 14, 2023

This PR #6601 introduced Factories::cells($class). Before it, the code was new $class().
It seems the Factories was introduced to load a short name cell class.

class_exists($class) ? new $class() was introduced because the Factories was not able to load the cells with the same short name even if we passed FQCN. (But the Factories has been changed, and now we can load them).

We just did not need to share the instance created by the Factories?
That is, we should have added ['getShared' => false] from the beginning?

@michalsn
Copy link
Member Author

class_exists($class) ? new $class() was introduced because the Factories was not able to load the cells with the same short name even if we passed FQCN. (But the Factories has been changed, and now we can load them).

Ok, I didn't know that was an issue.

Seems like we can now use Factories exclusively - just without sharing the instance. That should cover all the options.

@michalsn
Copy link
Member Author

I added an alternative solution: #8330

@kenjis kenjis added the documentation Pull requests for documentation only label Dec 14, 2023
@michalsn
Copy link
Member Author

We went with an alternative solution.

@michalsn michalsn closed this Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests for documentation only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: View Cell Inheritance Issue in Placeholder Values
3 participants