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

BUGFIX: Support empty dimension values for default fallbacks #156

Closed
wants to merge 4 commits into from

Conversation

hlubek
Copy link
Contributor

@hlubek hlubek commented Oct 23, 2015

Implement support for empty dimension values (for any number of
dimensions) of a node variant. This allows to have nodes like /sites
that will always be found, independent from the dimensions of the
context.

Another case is the transition to new dimensions that will be much
easier because existing nodes are already visible.

NEOS-1633 #close

Implement support for empty dimension values (for any number of
dimensions) of a node variant. This allows to have nodes like ``/sites``
that will always be found, independent from the dimensions of the
context.

Another case is the transition to new dimensions that will be much
easier because existing nodes are already visible.

Resolves: NEOS-1633
@kitsunet
Copy link
Member

kitsunet commented Dec 2, 2015

Looks quite good already

@kdambekalns
Copy link
Member

@kitsunet Quite good? So what is missing to simply good?

@kitsunet
Copy link
Member

I don't know, it has a WIP flag... Performance check would be good I guess.

@kdambekalns
Copy link
Member

Ah, true - what about the WIP label, @hlubek?

@hlubek
Copy link
Contributor Author

hlubek commented Dec 11, 2015

Well, that's why it has a WIP label. So if someone can do some performance tests with larger data sets and roughly compare the performance before and after that would help to see if we introduce a regression here.

@kitsunet
Copy link
Member

I currently compare master to kitsunet@8e7a206

I can check this in there as well...
In there is a 7000 nodes test installation...

@kitsunet
Copy link
Member

My change could easily support this feature here as well, I ust didn't add it yet.

@kdambekalns
Copy link
Member

Ah, so you wanted the "Needs feedback" label… :)

@skurfuerst
Copy link
Member

@kitsunet so is this change good to go performance-wise or not? :)

@kitsunet
Copy link
Member

Didn't come around checking yet and thought it was a bit late for 2.1 anyway so didn't see any reason to prioritize this. Will try ASAP and give more feedback.

@skurfuerst
Copy link
Member

OK, fine, so no need to hurry when we won't include it in 2.1 :)

@kdambekalns kdambekalns changed the title [FEATURE] Support empty dimension values for default fallbacks FEATURE: Support empty dimension values for default fallbacks Feb 17, 2016
@kdambekalns
Copy link
Member

Ping…

@hlubek
Copy link
Contributor Author

hlubek commented Feb 17, 2016

...Pong

Question is if we accept the performance penalty of the nested query (which is of the worst kind by referencing each selected row from the outer query) to fix this.

@kitsunet So what about your change of doing the reduction directly in the database? Do you think this behavior could be implemented with it? This is the better direction to me since the current state of querying too many nodes and reducing in PHP was always a workaround.

robertlemke added a commit to robertlemke/neos-development-collection that referenced this pull request Mar 2, 2016
This fixes an issue for sites with single or multiple dimensions which
have no fallback defined. When trying to translate content from an
existing dimension to a variant which does not exist yet, the
`NodesController` will fail with a fatal error due to the missing
Site Node in the respective dimension.

The solution in this change works for the given situation. A more
generic approach may be implemented as part of neos#156.

NEOS-1786 #close
@kitsunet
Copy link
Member

kitsunet commented Mar 2, 2016

Can defininitely be implemented, I think it even is already!

robertlemke added a commit to robertlemke/neos-development-collection that referenced this pull request Mar 2, 2016
This fixes an issue for sites with single or multiple dimensions which
have no fallback defined. When trying to translate content from an
existing dimension to a variant which does not exist yet, the
`NodesController` will fail with a fatal error due to the missing
Site Node in the respective dimension.

The solution in this change works for the given situation. A more
generic approach may be implemented as part of neos#156.

NEOS-1786 #close
@kdambekalns
Copy link
Member

Bi-weekly ping… :)

@robertlemke
Copy link
Member

For me this change works, the tests succeed and it could be merged into current master. I didn't test the performance yet though.

@kitsunet did you have a chance to check the performance? Or any further plans for this?

@kdambekalns
Copy link
Member

So, two questions:

  • Is the performance impact so bad it blocks the bugfix?
  • And should this go into master or 2.2 or even 2.1? It is a bugfix, and the original discussion is from fall of 2015, after all.

@kitsunet
Copy link
Member

kitsunet commented May 1, 2016

Nah, performance is ok

@kdambekalns
Copy link
Member

kdambekalns commented May 2, 2016

So, @robertlemke created another test and will add this, depending on that we decide where to include it.

This adds a test which exploits a bug where the site node can't
be found by traversing the parent nodes of a given node with
non-default dimension values in a multi-dimension set up,
when the root line has "gaps". In that sense "gap" means,
that not all nodes leading to the site node exist exactly with
dimension values like the "current" node.
@robertlemke
Copy link
Member

robertlemke commented May 2, 2016

Okay, I pushed the other test. For me this looks good now, it at least fixes a nasty bug we currently have in released and unreleased versions. I am sure that there are more fixes needed, but they can be provided as separate PRs.

@robertlemke robertlemke changed the title FEATURE: Support empty dimension values for default fallbacks BUGFIX: Support empty dimension values for default fallbacks May 2, 2016
@kdambekalns
Copy link
Member

I'll apply this to 2.2 and run all tests again.

@kdambekalns
Copy link
Member

Just created #498 against 2.2

@kdambekalns kdambekalns closed this May 2, 2016
skurfuerst pushed a commit that referenced this pull request May 3, 2022
TASK: Introduce Composer-based patching
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants