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

EZP-22191 : load always available content when using translation filter #687

Closed
wants to merge 3 commits into from

Conversation

bchoquet-heliopsis
Copy link
Contributor

https://jira.ez.no/browse/EZP-22191

I don't know how to implement integration tests on this.
@andrerom, @pspanja could you guide me or take over ?

@bchoquet-heliopsis
Copy link
Contributor Author

Well I'm not sure I'm responsible for every failure in the travis build (see the eng-GB vs eng-US ones) or am I ?

@andrerom
Copy link
Contributor

Integration tests are actually quite simple once it is done once, instructions for running them are in the readme, and for where to put it see for instance commits in this folder. In essence you just have to use the API similar to how you did when you stumbled upon this bug, it should fail before this commit, and pass after like other tests. Main thing to look out for is if solr tests fail, right now all of them skip because of regression from lazy services, so just be prepared to rebase when that is solved and then we can see if fix is needed there as well.

But essentially it would be easier for us to take over if there is integration tests, even just that.
And yes, it does look like the change breaks quite a lot of tests, so might not be the right way to solve it.

@bchoquet-heliopsis
Copy link
Contributor Author

OK thanks, I'll focus on the integration test and let you take over for the actual fix. As said in my bitter comment in #689 I'll circumvent this bug for the time being.

@pspanja
Copy link
Contributor

pspanja commented Jan 21, 2014

I'm not convinced this is the right way to solve the problem.
While it is already possible to achieve the same using Search, PAPI should make possible to choose when always-available is taken into account. Method that would take always-available into account in all cases could then be implemented in layer above.

So I would suggest that we add additional parameter $alwaysAvailable = false, which would be backwards compatible. That would also bring us step closer to implementing official Site API.

@docb22 what is your opinion on this?

@bchoquet-heliopsis
Copy link
Contributor Author

I created a test file, but I'm not sure I properly test exceptions. I can see the topic is far from being closed so I'll let you guys take a decision on what is to be done. Hope this PR will be useful in the end.

@ezrobot
Copy link
Contributor

ezrobot commented Jan 21, 2014

This Pull Request does not respect our PHP Coding Standards, please, see the report below:

FILE: ...Publish/API/Repository/Tests/Regression/EZP22191AlwaysAvailableTest.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 44 | ERROR | File must not contain multiple empty lines in a row; found 2
    |       | empty lines
 52 | ERROR | File must not contain multiple empty lines in a row; found 2
    |       | empty lines
--------------------------------------------------------------------------------

@andrerom
Copy link
Contributor

ping @lolautruche As this is kind of related to the one your working on ;)

$this->dbHandler->quoteColumn( 'language_code', 'ezcontentobject_attribute' ),
$translations
),
$query->expr->bitAnd(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment here explaining what you're doing ;-)

@lolautruche
Copy link
Contributor

@pspanja I don't see how it's wrong to fix it this way. Maybe this has to be better integrated in the BL, but this fix is perfectly valid IMHO.

+1 with a comment added

@andrerom
Copy link
Contributor

@lolautruche Seem all the failures on unit and integration tests?

@lolautruche
Copy link
Contributor

Of course my +1 is on the principle of the fix. Tests need to be fixed as well 😄

@andrerom
Copy link
Contributor

With so many failing tests, it seems to hint that the fix is not the correct way to fix it, I assume that is why @pspanja said so.

@pspanja
Copy link
Contributor

pspanja commented Jan 28, 2014

@lolautruche, @andrerom my comment is about the case when user is not interested in always-available fallback, and expects a NotFoundException when Content has none of the requested translations. It should be possible to control that, OTOH behaviour implemented here is natural to eZ and is something that should be provided in Site API (planned "higher level API").

In any case at the very least API and SPI methods docs need to be updated to explain always-available and we need more tests.

@bchoquet-heliopsis
Copy link
Contributor Author

I have to say I'm amazed to see how such a core feature has been overlooked until now.

I'll try to summarize how I think this should work, sorry if I state the obvious.

From an editor perspective, checking that box means they want this particular content to be shown wether it is translated or not. @pspanja : for that very reason it should be done by default, without having to explicit it.

That being said, this can be done at 3 levels I think:

  • at persistence level
  • at API level
  • at view controller level (I put the ViewManager in this category)

I tried to fix this at persistence level, I can see now this may not be the best fit.
I think filtering only at view controller level would not suffice as it'd mean developers would have to think about it each time they load contents.
I understand it could be a good thing to let API consumers bypass this flag in order to load a specific translation, and only that translation so maybe adding an optional $ignoreAlwaysAvailable parameter to content loading functions at API level would do the trick.

What do you think, and how would it work with children loading (another basic feature which needs some love ;) ?

As for this PR, I propose to close it and create a new one with its regression test to start anew, what about that ?

@andrerom
Copy link
Contributor

@pspanja I tend to agree with it being responsibility of persistence level, but you have a point we might benefit from having a flag for it. Could you sync with @docb22 and come up with a proposal / acceptance-of-this-as-is?

@pspanja
Copy link
Contributor

pspanja commented Jan 28, 2014

@andrerom Yes, I'll see how we can proceed from here.

@bchoquet-heliopsis From site dev perspective I would agree, but I would still go from simpler to more complex case, besides that we have BC concerns to think about as well. So by default it should behave the same it does now.

For now IMHO this can be closed, I'll keep you updated.

@bchoquet-heliopsis
Copy link
Contributor Author

@pspanja talking about BC, from a higher level, the lack of always available handling looks like a regression from 4.x ;)

@docb22
Copy link

docb22 commented Jan 28, 2014

-1.

  1. This functionality can easily be done above the public API. There is a languageCodes property in the current version info to see which languages are available.
  2. The language filter was only designed for avoiding loading to much unneeded data e.g. you have content with 20 languages and huge text parts in each language and it assumes that the caller knows exactly which language he wants to load.
  3. Returning a different language than passed in the language filter is a priori magic and hard to explain.
  4. In general: The PAPI was never intended to be used directly for making a website. The PAPI must serve different use cases. So here a convenient method which manages the fallback if desired has to be done in the appropriate layer above -> here we think about a Site API which could have a loadContent method which automatically manages loading content with the correct language for the site. Lets start it with this method! ;-)

@bchoquet-heliopsis
Copy link
Contributor Author

@docb22 you've got a mysterious username and are very convincing, are you some kind of mogul ? :)

@lolautruche it seems to me that this both confirms and contradicts what we did in e63fda9#diff-98e983e3e3385f1db184726a7e8f5cefL256

  • confirms because content validity should be handled at controller level
  • contradicts because instead of filtering on languages, we should have loaded the language and then check if it could be viewed

@andrerom
Copy link
Contributor

@docb22 I agree on the filter being explicit, but may we agree on the following:

  • Add a way to specify in language filter (or third param) that we also want to honor always available flag?

@docb22
Copy link

docb22 commented Jan 28, 2014

@andrerom not sure about if this will make life easier.

public function loadContent( $contentId, array $languages = null, $versionNo = null, $useTheAlwaysAvailableFlag = false );

If the passed language exist then the method behaves like without the flag.

If the passed language does not exist and if passing true and if the always available flag is set, the main language is returned in the content.

If the specified language does not exist and if the always available flag is not set, a (new) LanguageNotFoundException is thrown. (NotFoundException would not fit and be ambiguous).

But if i specify 2 or more languages - one exists and one not and the content has always available set to false what is returned then?

The documentation seems to grow exponentially for this method because it covers to much usecases. Therefore i recommend to see this method more low level and make the fallback logic above.

When making a Site API method the documentation is simpler as it only covers one usecase - loading the content in the correct language for a site (with exactly one language or with a not found).

The only thing to consider is that the site API implementation will make two calls - loading the version info and then it throws or loads the content fields with the correct language. but i would accept that.

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