@diegopino Sorry. This was by no means meant to be any offense. Just wanted to be helpful.
OK, i needed to read the source in order to really grok it. If i get it right, it's removes an obsoleted internal type hack, so my +1 for that.
Review: Code looks straightformward: Remove that hack, adjust tests.
I would RTBC that.
Strange JS test failure though - https://git.drupalcode.org/issue/drupal-3554720/-/jobs/7076303
NW for that. Or to scrutinize and confirm unrelated (can be RTBCed then imho).
You may want to learn and do a git blame, and add information what commit / issue brought that change.
Yes right. Code: \Drupal\translation_bliss\Drush\Commands\Extraction\TranslationExtractionCommands::extractAllFilesCommand
Great that you fond time to check it out. After i read the other issue, i knew you'd like it.
Of course this module is kind of visionary, in the sense that it dares to think different.
Which has its challenges to explain.
Yes, feel free to add documentation pages and experiment with ways to explain.
I am too deeep into that stuff to do this.
Also i am very short on time, i have really no bandwidth for evangelization.
But well, if word spreads, one agency after the other, things can change.
Ah yes of course you are right! And tests are green, perfect.
Maybe the kernel test can also be converted to a unit Test, but that is minor.
So thumbs up from me, and i guess the next step is integration into the field logic.
I have objections, likely remediable. No time now but will schedule time tommorrow.
@oily/andrewfarq:
I regret that my 'guesswork' verb hurt you.
Thanks for your explanation why you pushed that commit.
What i am reading is that you refer to catch's comment in #24.1 for the change you pushed.
If you read and understand answer #26, you see that that the previously mentioned comment is obsoleted.
Which catch himself acknowledged in #27.
So here we have the situation that the some person pushes a commit on the basis of some quite old comment, obviously w/o reading and unserstanding the conversation at that point in time. Which may give the chance of 'earning' contribution points, but impediates other people's work. Please replace the word guesswork with a rejecting idiom for such behavior on the basis of Drupal cooperative conduct that feels more appropriate for you.
I assume that you did what you think is right in good faith.
I ask you to be more thorough in the future, for the sake of respecting other people's lifetime.
No hard feelings on my side.
Gosh yes, \Closure is final, so berdirs BC point is strong.
So there are two ways,
- Add sth like a SerializableServiceClosure w/ an explicit opt-in (so no bc break), that gives us serialization for free, at the price of another Drupalism
- Find a way to identify service closures (or say the current recognition is good enough)
One idea to identify service closures: I once looked in a closure serialization library (crazy voodoo!), and the reflection class of a closure does have getSourceFile / getSourceLine (which will be ContainerBuilder in our case). This way - if it works out - would work with any way of defining the service closure, be it attribute or yaml.
@catch See #31 for the RTBC comment.
Test is green, modulo a spellcheck cleanup bug.
Test-only is red the expected way.
@oily/andrewfarq:
>Carried out code suggestion of @catch.
I cah not see how this relates to anything currently mentioned.
It is totally erratic and breaks the whole MR.
>2x unit tests are now failing but I think unrelated.
The breaking tests are the most related ones we have.
Note that guesswork might give you the chance of earning by-chance positive contribution points, but complicates the constructive work of others and gives you bad bad karma. Please restrain from doing that.
Reverted.
Reviewed.
Code looks solid and straightforward.
Tests are there and specific (test-only pipeline is red in the expected way).
Thought a while though about the algorithm that identifies serivce closures. It's "any 0-param closure that returns an object that the reverse container identifies as a service". Can there be false positives? Can the calling of all closures found in properties have side effects?
Both: Extremely unlikely, but YES.
The proper fix is straightforward: Use an introspectable and serializable ServiceClosure class in autowiring, and get serialization for free.
This approach also reduces the serialization / unserialization overhead, and makes it zero when all dependencies are such serializable ServiceClosures.
(Memo to me: Think about the same pattern in ModuleHandler: Make hook callables introspectable by invokers.)
Thx @godotislate, i like this feature a lot. Will review.
Huh, there is much confusion in the air, let's try to un-entangle it.
@catch #30: We have been much further in the discussion, please re-read #26 / #27, which explained this already, AND why this is just a special case of InterfaceLanguage / ConfigLanguage deviation, and that a CL cache context is needed. Also, the ConfigLanguage deviation is technically not a race condition, but a (questionable) design choice.
As of tests, #13 and #21 prove that IL and CL cache "context" are covered (in serial order).
RTBC as of #23, #24, #27, #29.
Also we agreed, that this MR is just a quick-fix to be replaced by memory cache (and in the meantime 🌱 [policy] Standardize how we implement in-memory caches Needs work got in).
I created all follow-ups mentioned in #26:
-
📌
Creata a ConfigOverrideLanguageCacheContext and use it in LanguageConfigFactoryOverride
Active
-
📌
(PP-1) Add a proper memory cache to EntityTypeBundleInfo
Postponed
-
📌
Add sth like Config::get($name, $language)
Active
-
📌
Fix ConfigurationLanguage deviating from InterfaceLanguage by setting it instantly
Active
Puh, it is done!
geek-merlin → created an issue.
geek-merlin → created an issue.
geek-merlin → created an issue.
geek-merlin → created an issue.
Looked at the patch and i looks like great and solid work! Preliminary notes:
- The API expectation where an entity is a ContentEntity and where maybe not and why did not appear clear to me
- The assumption that the different entities are instances of the same entity should be made explicit and maybe assert statements added
- That the language of the local entity is applied to all the others (except result entity) should be made explicit in the docs
- The conflicts array deserves a value object (immutable, withers, withouters)
Ah yes, i remember. Then this works with whatever that ID is. And if this issue is implemented, a bundle is guaranteed.
Wondering, what is the planned upgrade path to populate the "provider" field?
Yes, i still see this as a valid issue with no solution.
>... cache namespacing wrapper
+1 for that. Symfony has it too.
But i see no reason to not add it in a followup. Or do i miss sth?
Is this the right tag? Correct me if not.
Yes! Just cried wolf because I do not know enough about the cost of doing that later.
Feel free to PM me if time runs out and sth needs to be reviewed.
> ::getConfigOverrideLanguage() always calls ::getCurrentLanguage() with no arguments, which always returns the current interface language
@catch: Yes, i belived this too, but it's a myth.
One thing is the difference between language negotiation and LanguageRequestSubscriber, and in the meantime a lot can happen (we can change this by making interface language negotiation setting ConfigLanguage instantly, but the current way of working might have reasons, and a change might have unforseeable consequences).
It's also that there are a lot of places where ConfigLanguage is **intentionally** changed. Look e.g. at ConfigurableLanguageManager::getNativeLanguages. It changes ConfigOverrideLanguage to retrieve the respective translations. And there are a handful of other places that do it like this.
In a well designed API, there would be sth like Config::get($name, $language). But there is not, and so messing w/ global config language is the only way of getting the result.
And yes, this means essentially that there **should** be a ConfigOverrideLanguageCacheContext. And indeed, in TranslationBliss module we maintain this cache context for exactly this reason.
And yes, you are right, a proper memory cache (do we need variation cache?) would be the "proper" thing and i can add a followup for this.
I have published the CRs.
@amataescu, @catch:
Please bear again with my thoroughness. Can you look at this: 📌 Make provider the bundle key of workspaces (or document why not) Active
geek-merlin → created an issue.
Great! All review items are resolved. Nightwatch failure looks unrelated.
Banzai!
A small step code-wise, but...
geek-merlin → created an issue.
Great you appreciate it. The finishing line is in sight, i suppose it is RTBC if these ones are addressed.
Code (contains some unrelated cleanups but) LGTM!
Tests are in place.
Really nice work Andrei! Esp. the change records play in the top liga.
Left some more comments. Some of them drilling deeper on having the new API correct and clean.
Imho yes, despite limited resources.
@vitalii: Thanks for working on this! I really appreciate this, as i'm so out of time.
Thoughts:
- You have experience with sprintf? In fact the code you suggest should be equivalent to the existing.
- Can you push your WIP commits, even if they are very WIP? Then i can give feedback much better.
- Con you add a UnitTest with a dataProvider that tests the TwigScript compile step? We need it anyway and it will make it dead clear what happens.
Really nice work! Left some code comments.
Current test failure looks like it needs a simple test expectation update.
Chapeau! From a first glance this looks exactly like i envisioned.
My gut feeling is one iteration to weed out some nits to RTBC.
Setting to major task, as this is an important step towards important use cases (ForwardRevisionNG, DomainNG, NodePreviewNG, ...)
Summary (maybe CR draft):
Every workspace now has a provider
Every workspace now has a provider. To create a provider, create an autoconfigured service extending from WorkSpaceProviderBase.
WorkSpaces module Provides its functionality now via the WorkspaceDefaultProvider service (w/ ID 'default').
Will start a code review now.
I doubt that "make json validation code simpler" is a valid reason for deprecating an important feature.
I know that there is a pseudo-religious camp essentially saying "no objects in twig".
Otoh there is Attributes object.
AND, there are large codebases depending on ViewModels, which help keep complex codebases clean.
This concept is essentially "documented API instead of obscure ArrayPi".
Patch is trivial and removes a log that should not be logged. I suppose it does not need a test, and similar patches do not.
Tests are green. Setting rtbc per #4.
Looks like a good start! Natural next steps (and imho all we need here) are:
- Change the list format to sth like this:
- format: - Add description here if there is one.
- Filter out irrelevant properties: isInternal(), isReadOnly(), isComputed() (which here kicks out the processed-properties).
#31.2:
Yes i wondered too whether the php attribute should live in a "PhpAttribute" dir, as we have a html attribute class called Attribute. I decided to go with the quasi-standard dir name. If maintainer or committer see it differently, it's a trivial change to make.
geek-merlin → created an issue.
@ressa: Your train of thought in the IS was exactly mine.
And i guess you also experienced that Drupal gatekeepers have their beliefs, und usually are extremely conservative on that.
Whether this is good for diversity and the future of Drupal is a discussion beyond this one.
That said, i had the same vision and now there is a module for that:
https://www.drupal.org/project/translation_bliss →
I'd love to have you and your agency in the club of pioneers. Feel free to PM me.
Yes, traits have landed for a long time, this can be closed.
Yes it is. The interface name changed, the problem persists.
The implemntation of OptionsProviderInterface should move from FieldItem to FieldDefinition, with deprecation dance.
@rgpublic:
> Thanks for the implementation!
Nice you like it. Maybe you want to review and RTBC.
> And, looking at the issue summary, we still don't protect properties, right?
> I guess this would be only about writing to them. Is this actually possible in twig?
You are right (yet!), so that point is minor and i removed it from the IS.
> Do we also need to allow this on whole classes? That could eventually replace the somewhat arbitrary special case for HTMLAttribute...
> Or, do we force developers to use it on any individual method - could be safer, I don't know.
I suppose the latter.
That said, i decided to focus on the first step with a huge quick-win, and postpone ALL other changes and bikeshedding discussions about deprecation paths to a followup.
The update path may be minimal:
- Read in the yaml including tokens
- Generate an array twig expression
- ...where the [query:foo] tokens are converted to query.foo expressions (and quotes are removed)
- Any not recognized tokens are reported in the update message for manual handling
And of course this is just an extra chore and can come in a followup.
I do not see any need for tokens when everything you need is in the twig context. We might add an alter hook so people can add special data they need, but the bread-and-butter use case needs only the query string.
Did i miss sth?
Implement first POC code w/ verbose comments.
Separating the 1.x / 2.x approaches, leanoÃng toward 2.x.
geek-merlin → created an issue.
Implementation is dead simple. NW for tests though.
geek-merlin → created an issue.
There are 2 possibilities fora TwigScript API:
- Return stuff via a magically named variable.
- Return stuff via a dedicated object (like psr16 events) with setters.
The latter is dependent on related core issue.
geek-merlin → created an issue.
Now that there is hope to get this module to a proper state, this sparked a lot of concept thoughts in my brain.
Current braindump:
- Yes, this is the direction to go.
- I have a concept where we can have all the twig bliss w/o the json_dump technicalities (will roll it into poc code asap)
- The twig approach needs different configuration, BUT can be autogenerated by a simple update script
- So let's roll a 2.x version based on this approach
- ...and do some cleanup also (drop old core and php versions, move hooks to classes, etc.)
Now we got it.
New test alone is red
Fail 0.011s testGetAllBundleInfoCachesPerInterfaceAndConfigLanguage
https://git.drupalcode.org/issue/drupal-3446084/-/pipelines/624344
With fix it's green.
composer patch attached for convenience.
Anyone to apply and rtbc?
Yup, it's red.
Fail 0.013s testGetAllBundleInfoCachesPerInterfaceAndConfigLanguage
https://git.drupalcode.org/issue/drupal-3446084/-/pipelines/624306
Test-only is red
Fail 0.015s testGetAllBundleInfoCachesPerLanguage0
https://git.drupalcode.org/issue/drupal-3446084/-/pipelines/624295
(will adjust test name in the next commit anyway)
There is a problem that during the request, config override language and interface language may be different.
Not only that config language is set "late" by a request listener, config override language can be switches intentionally, and this is cone in quite some places.
In #10, i thought this es a separate problem. I was wrong.
Because: A bundle label may be a t() result, or a bundle entity config string, so ETBI depends on BOTH language settings.
In an ideal world, bundle info has an explicit cache context. Until then, let's hard code this.
@tstoeckler #22
Which essentially means to generate lazy instead of bulk.
#1 for best suggestion so far.
As of escaping.
epp config:
value: "[query:foo]"
and suppose you pipe into query.foo (with proper url encoding...):
'my-value"\nanother_property: "evil-injection'
I guess you get the idea, and there have been such issues.
That's why i want to get rig of this to not lead people into WTF.
I.e. deprecate old usage and roll a 2.0.0 w/o tokens and twig.
Postponing on the related IEF and Paragraphs issues. Please work on them, then this is simple.
geek-merlin → changed the visibility of the branch 8.x-1.x to hidden.
I closed that merge request because the hook_entity_field_values_init approach has no use as it runs only on create.
Thank you all for that idea!
As maintainer I thought a bit and made a plan for the future of EPP withregards to that.
The approach in the other issue is simpler and allows arbitrary complex processing and values.
Contributions appreciated!
geek-merlin → created an issue.