Skip to content

Conversation

D063520
Copy link

@D063520 D063520 commented Jun 18, 2019

This solves issue 16, i.e. a problem that occurs when adding statement that contain a unit. The idea of the fix is to replace the units urls with the corresponding local urls. My IDE somehow reformatted the whole file. if this is a problem I can change that.
Salut
D063520

$localId = $this->entityMappingStore->getLocalId($entity->getId());

if ($localId && !$this->statementsCountLookup->hasStatements($localId)) {
$this->statementsImporter->importStatements($entity_new);

Choose a reason for hiding this comment

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

I would go with $camelCase

Copy link
Author

Choose a reason for hiding this comment

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

yes, ok

}
$snakList_new = $statement_new->getQualifiers();
foreach ($snakList_new as $key2 => $snak_new) {
if ($snak_new instanceof PropertyValueSnak) {

Choose a reason for hiding this comment

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

You can shorten this our by flipping the condition and doing a continue inside or extract this to a method. This is too much indentation

Copy link
Author

Choose a reason for hiding this comment

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

what you mean by flipping the condition?

Choose a reason for hiding this comment

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

I mean something like this:

if (!$snak_new instanceof PropertyValueSnak) {
confinute;
}

$referencedEntities = $this->getReferencedEntities($entity);
$this->importEntities($referencedEntities, false);

$entity_new = $entity;

Choose a reason for hiding this comment

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

Is it really needed to clone the entity and do the changes? Why not changing them in the place? Sorry if it sounds stupid, I don't know the full context here.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not a good php programmer. I'm not sure if this is working because the coded iterates over the statements and they are changed. If you think this can work I will try ....

Choose a reason for hiding this comment

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

hmm, good point. I think it can stay like this.

@Ladsgroup
Copy link

Ping :)

Copy link
Author

@D063520 D063520 left a comment

Choose a reason for hiding this comment

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

Was a bit busy .... and first time I'm doing this .... should I make a new pull request with the changes you proposed?

$referencedEntities = $this->getReferencedEntities($entity);
$this->importEntities($referencedEntities, false);

$entity_new = $entity;
Copy link
Author

Choose a reason for hiding this comment

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

I'm not a good php programmer. I'm not sure if this is working because the coded iterates over the statements and they are changed. If you think this can work I will try ....

$localId = $this->entityMappingStore->getLocalId($entity->getId());

if ($localId && !$this->statementsCountLookup->hasStatements($localId)) {
$this->statementsImporter->importStatements($entity_new);
Copy link
Author

Choose a reason for hiding this comment

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

yes, ok

}
$snakList_new = $statement_new->getQualifiers();
foreach ($snakList_new as $key2 => $snak_new) {
if ($snak_new instanceof PropertyValueSnak) {
Copy link
Author

Choose a reason for hiding this comment

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

what you mean by flipping the condition?

@Ladsgroup
Copy link

Was a bit busy .... and first time I'm doing this .... should I make a new pull request with the changes you proposed?

All good, no I think a commit on top of this would be enough.

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.

2 participants