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

merge pomm bridge with pommbundle #104

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

stood
Copy link
Member

@stood stood commented Sep 4, 2018

No description provided.

@sanpii
Copy link
Member

sanpii commented Sep 4, 2018

We probably need to add something in composer.json. conflict or replace ?

@stood
Copy link
Member Author

stood commented Sep 4, 2018

I add conflict but I have a pb with namespace I will to correct

@stood
Copy link
Member Author

stood commented Sep 5, 2018

It's good :)

$model_name = "${class}Model";
}

if (!class_exists($model_name)) {
Copy link

Choose a reason for hiding this comment

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

Might be worth to move it up a bit. As it doesn't require the session to exit.

break;
default:
$type = Type::BUILTIN_TYPE_OBJECT;
$name = $pomm_type;
Copy link

Choose a reason for hiding this comment

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

name seems unused. class instead ?

*/
public function normalize($object, $format = null, array $context = array())
{
return $object->extract();
Copy link

Choose a reason for hiding this comment

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

I would build an stdClass and forward it to a the normalizer again. (By implementing NormalizerAwareInterface)

/**
* @param Pomm $pomm
*
* @return null

Choose a reason for hiding this comment

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

return void

public function __construct($unused = null, Stopwatch $stopwatch = null)
{
if ($unused !== null) {
trigger_error("The parameter Pomm has been deleted for to delete the high dependency.", E_USER_DEPRECATED);

Choose a reason for hiding this comment

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

Maybe rephrase it to The Pomm parameter has been deleted in order to remove the higher dependency or somethin ?

* @param array $data
* @param $session
*
* @return null

Choose a reason for hiding this comment

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

void

/**
* @param string $name
* @param array $data
* @param $session

Choose a reason for hiding this comment

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

@param Session $session

if (isset($context['model:name'])) {
$model_name = $context['model:name'];
} else {
$model_name = "${class}Model";

Choose a reason for hiding this comment

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

I prefer the

$model_name = "{$class}Model";

syntax as it allows the use of sub properties or function calls

}

if (!class_exists($model_name)) {
return;

Choose a reason for hiding this comment

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

Would add a warning in the logs could be usefull here ?

if (isset($context['model:name'])) {
$model_name = $context['model:name'];
} else {
$model_name = "${class}Model";

Choose a reason for hiding this comment

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

same thing, I prefer the

$model_name = "{$class}Model";

syntax as it allows the use of sub properties or function calls

$model_name = "${class}Model";
}

if (!class_exists($model_name)) {

Choose a reason for hiding this comment

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

Might be worth to move it up a bit. As it doesn't require the session to exit.

if (isset($context['model:name'])) {
$model_name = $context['model:name'];
} else {
$model_name = "${class}Model";

Choose a reason for hiding this comment

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

same thing

"{$class}Model";

$interfaces = $reflection->getInterfaces();

// @TODO Use FlexibleEntityInterface::class with php >= 5.5
return isset($interfaces['PommProject\ModelManager\Model\FlexibleEntity\FlexibleEntityInterface']);

Choose a reason for hiding this comment

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

Couldn't we achieve the same result with is_subclass_of ?

@stood
Copy link
Member Author

stood commented Dec 8, 2019

After reflexion, I'm wondering if it's a good solution. In the futur, if we want develop a new Bundle with a new component Pomm (other ModelManager for example) we will to duplicate the code.

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