eca_endpoint: Action "Request: Get path argument" needs ability to ignore language code

I am working on this issue

Thanks for addressing this issue.

I briefly looked into your current MR and suggest considering a different solution path. Instead of brute-force checking whether we might have a language code as prefix, may only solve this particular use case but not anything else that may add a certain prefix to a given system path. Also, there even may be routes that could actually start with a language code as prefix but were not added by the translation system...

Here is a suggested way to get the "correct" path we want to finally work with:

$current_url = Url::fromRoute('<current>');

// We want to get the path without language prefix, therefore use internal.
$path = $current_url->getInternalPath();
$path_args = explode('/', $path);
// ...

I have updated the MR as per @mxh review comments

Thanks. You've removed the option "skip_language_prefix" in the last commit, but this option is needed - could it be re-added?

The new logic should also only apply when this new option is in use, otherwise the previously implemented logic should just be applied as it was before, so we don't break existing configurations.

I’ve re-added the skip_language_prefix option and updated the logic to use the internal path only when this option is enabled. Existing configurations will continue to work with the original logic.

Please have a look and let me know if any further changes are needed.

Thanks, now when using the internal path, we may want to use a proper naming of the option for that. Instead of "Skip language prefix" it may be called "Use internal path" as option, with a description something like "When using the internal path, added prefixes such as language code will be ignored. For example, both "/en/node/1" and "/node/1" would return "1" when index is set to 1.". This should make it more clear and also be understood for other cases than language code prefix.

Tests and code checks are currently all red, but that doesn't seem to come from the MR itself, rather from the current state of the 3.0.x branch. Once the section above got addressed we may set this again to Needs review.

Thanks @mxh for the detailed feedback!
I’ve updated the MR to reflect the suggested changes:
Renamed the option from "Skip language prefix" to "Use internal path".
Updated the description to make it more general, not just about language prefixes.
Please have a look and let me know if any further changes are needed.

Thanks for addressing the feedback.

The logic is still a bit different than before when we don't use the option. For example the $path parts are being split using the explode function and if we have not specified any path position, the following is being returned now: return '/' . implode('/', $parts);

Despite the logic not being the same anymore we may set this to NR for now as the end result may still be the same (but I'm not sure).

Note, I'm in the process of fixing all tests for the 3.0.x branch of ECA in 📌 Fix PhpStan 2.0 issues with the code base Active and the MR can be rebased when this has been completed. Then we can make sure that the MR passes all tests as well.

OK, the other issue is fixed and I've rebased the MR, let's see what the tests are looking like now.

There are some PHPCS complaints that should be addressed.

This is looking good to me, except for 1 suggestion that I've made in the MR.

Then, we need an update hook to update existing models and we should have some tests.

Note to self: this should be back ported to 2.1.x when completed.

@jurgenhaas I have updated the description text to use a clearer example, where the index and the value are different to avoid confusion.

Thank you @prabha1997 that's much better. It's now time for the update hook and tests, are you up for that?

I've given it a try, but I'm having some difficulty getting it to work as expected. I haven't been able to achieve the correct results yet.

@prabha1997 which part are you trying first? For the update hook there are examples in the 2.x branch, e.g. eca_update_8011(). For tests, you could probably also build on top of already existing tests, if that's helpful.

@jurgenhaas
In the 3.0.x branch, I noticed that the eca.install file was removed.
Since I need to add an update hook, is it okay to reintroduce the eca.install file, or is there a specific reason it was removed?

@prabha1997 the missing eca.install file is due to the expected upgrade path to ECA 3 where all the updates must have been applied before upgrading to 3. So, therefore it would be better for this to be a separate MR which goes against the 2.1.x branch.