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

Quick fix for association field. #11

Merged
merged 3 commits into from
Mar 4, 2017
Merged

Quick fix for association field. #11

merged 3 commits into from
Mar 4, 2017

Conversation

miguelpayet
Copy link
Contributor

A quick fix for language-processing for fields pulled in via association field. Tested only in one site and one section (that is, basically untested).

Look at non-language fields to see if they contain item element and put them through processEntries function.
Add a parameter to processFields function.

Apparently my editor screwed up the spacing in a bunch of lines I didn't touch. The changes are in 256 & 272 and then starting from 336.

…f they contain item element and put them through processEntries function
@miguelpayet
Copy link
Contributor Author

Jens: feel free not to merge this. Thanks for this extension, BTW.

@jensscherbl
Copy link
Contributor

Thanks, will have a look at this.

@twiro
Copy link
Owner

twiro commented Feb 15, 2017

This fix is a great help if you're combining multilingual and association field/output.

I have used this code in production on at least 4 different projects for about a year now and it seems to work stable and reliable - so I think it would be a good move to finally release it.

@jensscherbl - I just started working on some updates for this extension, but read on the forum that you're not planning to use/support it anymore. Which is sad as I really like the approach to multilingual you built here and think it's extemely useful especially for smaller multilingual projects (for which the "other" way of doing multilingual always seemed like a huge overkill to me).

If you really want to get rid of it I'd gladly take over ownership and take care about putting out new releases and possibly also extending the readme a little bit (information about frontend language detection and what this extension does and doesn't do regarding routing/urls would be super helpful... I know I struggled quite a little bit with this part when first working with the extension and most irritation reported in the forum seems to derive from these aspects too).

But If you changed your mind and are planning to keep this extension I'd gladly help updating it, explain/discuss my recent changes with you and send PRs for change that looks good to you.

Just let me know & thanks a lot for the great work!

Copy link
Owner

@twiro twiro left a comment

Choose a reason for hiding this comment

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

Hey Miguel,

I just adopted the "multilingual"-repo from Jens and would merge your PR after you made some little tweaks (see my comments) and resubmit the PR against integration-branch!

Thanks for your work and let me know if you're not interested - then I'll simply apply theses changes myself.

Kind regards, Roman

@@ -305,38 +305,44 @@ private function processFields(XMLElement $xml)
foreach ($elements as $element_index => $element) {

// get element handle
if (is_object($element)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you change this line for this:

if ($element instanceof XMLElement) {

like used in the findEntries() function above?

$xml->removeChildAt($element_index);
}
} else {
$test = $element->getChildrenByName('item');
Copy link
Owner

Choose a reason for hiding this comment

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

This line should be removed

}
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Indenting need to be fixed.

@@ -253,7 +253,7 @@ public function dataSourcePostExecute($context)

// datasource output entries

private function findEntries(XMLElement $xml)
private function findEntries(XMLElement $xml, $entries_name = 'entry')
Copy link
Owner

Choose a reason for hiding this comment

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

I think a variable named $node_name would make more sense than $entries_name as we're trying to process both entry- and item-nodes. Could you change this?

@@ -269,7 +269,7 @@ private function findEntries(XMLElement $xml)

// check if element is entry

if ($element->getName() === 'entry') {
if ($element->getName() === $entries_name) {
Copy link
Owner

Choose a reason for hiding this comment

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

should then be $node_name too...

@twiro twiro self-assigned this Feb 28, 2017
@twiro twiro added this to the 1.2.0 milestone Feb 28, 2017
@miguelpayet
Copy link
Contributor Author

miguelpayet commented Feb 28, 2017 via email

@twiro
Copy link
Owner

twiro commented Mar 3, 2017

Hi Roman, of course I'll make the changes and resubmit the PR. Thanks.

Thanks, Miguel! Do you think you can submit the changes in the next few days? Would love to get version 1.2 out as soon as possible to focus on the upcoming 2.0 update.

@miguelpayet
Copy link
Contributor Author

miguelpayet commented Mar 3, 2017 via email

@twiro
Copy link
Owner

twiro commented Mar 3, 2017

Great - Thank you! And no need to apologize...

@miguelpayet
Copy link
Contributor Author

Hi Roman, I updated the code as you suggested & pushed the changes to my fork of the repo. This PR is now showing the updated code.

@twiro twiro merged commit e370d16 into twiro:master Mar 4, 2017
@twiro
Copy link
Owner

twiro commented Mar 4, 2017

Thank you Miguel - I mergerd your PR and released version 1.2.0!

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