-
-
Notifications
You must be signed in to change notification settings - Fork 224
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
Conversation
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
Looks quite good already |
@kitsunet Quite good? So what is missing to simply good? |
I don't know, it has a WIP flag... Performance check would be good I guess. |
Ah, true - what about the WIP label, @hlubek? |
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. |
I currently compare master to kitsunet@8e7a206 I can check this in there as well... |
My change could easily support this feature here as well, I ust didn't add it yet. |
Ah, so you wanted the "Needs feedback" label… :) |
@kitsunet so is this change good to go performance-wise or not? :) |
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. |
OK, fine, so no need to hurry when we won't include it in 2.1 :) |
Ping… |
...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. |
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
Can defininitely be implemented, I think it even is already! |
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
Bi-weekly ping… :) |
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? |
So, two questions:
|
Nah, performance is ok |
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.
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. |
I'll apply this to 2.2 and run all tests again. |
Just created #498 against 2.2 |
TASK: Introduce Composer-based patching
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