PHP 8.2 compatibility for the 2.0.x branch

Hi,
Please review the MR!129

Changes looks good.

This is a duplicate of #3360426: PHP 8.2 compatibility. Which is further along in its work.

Perhaps this simple solution should be committed to the 2.0.x branch to get that release fully supporting PHP 8.2.

#3360426: PHP 8.2 compatibility has been committed to 3.0.x. Let's reopen this to make 2.0.x also PHP 8.2 compatible.

Manually tested on a project with https://git.drupalcode.org/project/facets/-/merge_requests/129

RTBC+1

Patch works!
Thank you

Fixes the issues for 2.x

I am using 2.06 of Facets and I too have just updated my PHP to 8.22 and I see the same error report which makes my site unusable :-) ...

Deprecated function: Creation of dynamic property Drupal\facets\Result\Result::$transliterateDisplayValue is deprecated in Drupal\facets\Plugin\facets\processor\DisplayValueWidgetOrderProcessor->sortResults() (line 72 of modules\contrib\facets\src\Plugin\facets\processor\DisplayValueWidgetOrderProcessor.php)....

I have applied (or at least attempted to) the patch from #10 but it did not make any difference - according to my local GIT, it made no changes to the...

"<site>/modules/contrib\facets/src\Result/Result.php" file?

Can anyone please advise here or do I just need to wait for the next release of the 2.x version?

Thanks

Another +1 for RTBC, the MR 129 fixes the error, and would be great to get in the module, to avoid flooding the logs with warnings.

@SirClickalot: You can quickly test if the patch works by downloading and applying it like this, revert with -R. Apply permanently with Composer (UPDATED after comment by @agoradesign, see below):

wget https://git.drupalcode.org/project/facets/-/merge_requests/129.diff
patch -p1 < 129.diff

This should be the only way to apply patches :) https://medium.com/appseed-io/apply-drupal-9-patches-with-composer-7b1e5...

Of course, always use Composer :) I added the example just to verify if it applies or not, and will add a note about that.

EDIT, feb 2024: I have now read the article "Apply Drupal 9 patches with composer" closer, and that is NOT how to do it. Apply it with Composer, yes -- but the only safe method is to download the patch, not hotlink it to a remotely hosted patch. For a safe method see Patching projects using Composer.

This patch fixes the error for me
https://www.drupal.org/files/issues/2023-08-16/3336646-function-is-depre...
Thanks!

Another RTBC +1. It would be great to get this in the 2.x branch.

Applying the patch https://git.drupalcode.org/project/facets/-/merge_requests/129.diff fixed the issue on my side as well

RTBC +1

The patch https://git.drupalcode.org/project/facets/-/merge_requests/129.diff worked for me! Thanks!

While this ad hoc patch will most certainly fix plugins provided by the Facets module itself, it is not extensible, in the sense that custom sort plugins will be unable to dynamically set properties on Result objects, so they will have to deal with this in a different way. It should be noted that at the time you feel the urge to write such a custom plugin, you generally follow the example of a plugin provided by the main module.

I do not know, maybe we could have a general purpose property, e.g. an array, or a public method to deal with a "bag" of attributes on a Result object? That way all plugins, whether they ship with the Facets module or another one, could follow exactly the same method.

Patch #2 is the wrong approach, #12 and the merge request are the correct approach.

@FMB: Classes that extend one class can add new properties to it, that's how OOP in PHP works. So if someone extends the Facets plugins and uses dynamic properties, all they have to do is define the new properties in their new class, it's literally that easy.

@DamienMcKenna: thanks for your answer. Sure, I am quite aware of that ^^ But when you write a sort plugin, you generally extend SortProcessorPluginBase, and most importantly implement SortProcessorInterface, which enforces a sortResults method, which takes two Drupal\facets\Result\Result arguments. The bottom line is you have no choice but to use a Result object (unless you inherit from this interface and so on...), and even if you could extend that one, it would not be particularly handy to deal with your own Result class when all you wanted was precisely to reuse the machinery provided by the module to implement a little business logic.

That's still unrelated to this issue and belongs in a support request.

I beg to differ. This is totally related in the sense that we should look for a way to adapt sort plugins to PHP 8.2, whether they ship with the Facets module or not. The proposed solution is ad hoc in the sense that it statically adds properties which are not directly related to what the object does, only providing a feeling of superficial "compliance" with what is now dictated by PHP.

I dealt with my own plugins the best I could, but IMHO it does not make sense that the same mechanism cannot be used as in the main module. Plus it is a source of errors for people who will take Facets plugins as an example in the future.

These are just deprecations to help you be aware of forthcoming changes to how PHP itself works. When PHP 9 is released and dynamic attributes are no longer technically possible, will it be the fault of this module's developers that someone else extended the classes and did so improperly, ignoring the deprecation messages? Using the AllowDynamicProperties property ignores the reality that the PHP language is changing and code needs to be updated.

Individual developers are responsible for the code they write, not anyone else.

The solution provided here is appropriate and RTBC.

I have already dealt with the code which is under my responsibility. Now, if you read my proposal in #25, you will see that allowing dynamic properties is not what I suggest.

Your suggestions are out of scope for this issue which is focused on making the module run on PHP 8.2 without errors or notices, feel free to bring up other topics in other issues.

adding a PS to #33 - it is both totally unrealistic and imho semantically totally wrong refactoring the architecture in branch 2.x to allow kind of dynamic properties in a future-safe way. So please FMB, feel free to bring in your ideas in a separate issue either for version 3.x or maybe a future 4.x branch, and anyone here will really appreciate your efforts, but as Damien already stated, this issue is about making the module PHP 8.2 compatible, so that we are not forced to switch to 3.x immediately, which caused me some problems a while ago. And the proposed patch does exactly that. It adds some properties, and doing this does not make anything worse regarding class inheritance.

imho the only thing that is discussable in that matter, that we should think about not only adding the two class properties as proposed, but also add the AllowDynamicProperties PHP attribute, so that any custom extensions are safe regarding this matter in PHP 8.2

Either way we decide, this is RTBC and will hopefully land soon :)

I was not aware of this, but now you mention it @agoradesign, such a mechanism as I was trying to describe already exists on 3.x, thanks for pointing this out, it does make perfect sense now.

In totally unrelated news, I suggest to use this link for the patch https://git.drupalcode.org/project/facets/-/merge_requests/129.diff?diff... (the diff_id freezes it in a given state over time).

oh yes, you're right. didn't know that too... ok, then everything's fine now :)

@FMB - Can you please enlighten us on how to find the diff_id? That looks super useful, but I'm digging through the page source and gitlab docs and have no idea how to find it.

@mortona2k 129 is the id of the merge request ;)
Basically you just need to click on the merge request and add ".diff" to the url

Thanks @MacSim, but it's not clear where diff_id=648306 can be found ...

• Add possibility to download .patch/.diff from merge request versions
• Add support for diff_id in links to file in merge requests

@ressa Sorry I didn't realised that second id.
I don't know where he found that... I might be wrong but I would say you don't need it
The diff is available here https://git.drupalcode.org/project/facets/-/merge_requests/129.diff and it's the same than https://git.drupalcode.org/project/facets/-/merge_requests/129.diff?diff... ;)

Yes, the patches look the same, I am not sure it's possible to pin a .diff file to a specific commit point in history, yet. The problem is that the patch MR content can change on each commit, and that's a security risk:

from https://www.drupal.org/docs/develop/using-composer/manage-dependencies#p...

Also, I have now read the article Apply Drupal 9 patches with composer closer, shared in #19, and that is NOT how to do it. Apply it with Composer, yes -- but the only safe method is to download the patch, not hotlink it to a remotely hosted patch.

Can you please enlighten us on how to find the diff_id? That looks super useful, but I'm digging through the page source and gitlab docs and have no idea how to find it.

@mortona2k sure, go to the changes tab for this MR. Under the tabs and before the code, it says "Compare 2.0.x and latest version". Click on "latest version", and then again on "latest version" in the dropdown menu that appears. You can now see the diff_id in the URL: https://git.drupalcode.org/project/facets/-/merge_requests/129/diffs?dif.... At this point, just replace "/diffs" by ".diff" and there you are.

True, you may commit patches in your own repository, but specifying a diff_id makes it difficult to turn the patch into malicious code later on.

@FMB Thank you very much.

I generally use a MR diff as a patch and downloaded it to /patches. But my patches were named after the MR ID, and I had no way to tell if there was an update or keep track of previously working versions.

People often upload patch files to a thread with the comment name. I think using the MR + diff_id would be much more useful for identifying where it came from.

If Drupal.org/Gitlab could add the diff_id to the patch link in these issues, that would help people linking directly to the diff avoid getting bad commits added to the branch.

@FMB: Following your steps, I get the same patch, which is the latest:

Compare 2.x and latest

• https://git.drupalcode.org/project/facets/-/merge_requests/129/diffs?dif...
• https://git.drupalcode.org/project/facets/-/merge_requests/129.diff?diff...

Compare 2.x and version 1

• https://git.drupalcode.org/project/facets/-/merge_requests/129/diffs?dif...
• https://git.drupalcode.org/project/facets/-/merge_requests/129.diff?diff...

Both 129.diff?diff_id=648306 and 129.diff?diff_id=419569 result in this, where version 1 (diff_id=419569) should be protected $transliterateDisplayValue;:

diff --git a/src/Result/Result.php b/src/Result/Result.php
index 334dc68559ba310b8743a188a66a2f1075e059b1..6ae88d99d980745766da700f8e2eb5eef779ebfa 100644
--- a/src/Result/Result.php
+++ b/src/Result/Result.php
@@ -73,6 +73,20 @@ class Result implements ResultInterface {
    */
   protected $children = [];
 
+  /**
+   * The facet transliterate display value.
+   *
+   * @var string
+   */
+  public $transliterateDisplayValue;
+
+  /**
+   * The term weight.
+   *
+   * @var int
+   */
+  public $termWeight;
+
   /**
    * Constructs a new result value object.
    *

I tried with another MR with more commits. Turns out you are right, this works for the /diffs page but apparently not for the patch URL (.diff), my bad. Thanks for investigating this.

Thanks for confirming, let's hope Add support for diff_id in links to file in merge requests will add this before too long:

If you switch between different versions of a merge request you won't be able to link to a file or a file line for this specific version. The link to the file or the file line always points to the latest version. We should incorporate diff_id parameter for these links to support linking to different versions of the MR in files.

Copy of patch from https://git.drupalcode.org/project/facets/-/merge_requests/129.diff attached, to be used with composer, see "Patches from drupal.org merge request URLs are dangerous?".

RTBC, should be a easy merge. Can we get this in? LGTM

Thanks for the update ;)